Rethink changeSupply

[DanielVF]

PR posted for discussion.

This PR makes several subtle changes to one of the most fundamental methods of OUSD. While I think all of these changes are in the direction of simplicity and correctness, it certainly behoves us to think this PR through.

Rounding

The old method "rounded up" the rebase ratio, making rebasing accounts having tiny amounts more than they would otherwise have. (The actual division operation rounded down, but since a smaller rebasingCreditsPerToken is more money, this was effectively rounding up the balances.) The size of this error was small - adding a billion billion dollars to the contract would result in being off by a dollar. The size of the error would increase as OUSD earns yield though

Because of this rounding error OUSD's totalSupply would almost always be set to a value greater than what the vault sent it.

This rounding error also means that if a rebase came in near the MAX_SUPPLY, the rounding error would push the recorded supply over MAX_SUPPLY.

I've change the rounding to effectively round the rebase ratio down.

I went for a simple approach here on the rounding - just adding "1". This is technically incorrect in the super rare case when it the numbers would actually get the right answer. I'm open for discussion on this. I've not updated the two tests that this causes to fail.

Total Supply Calculation

There's roughly three properties we'd like the new total supply to have:

• Match the sum of all OUSD accounts, or at least be above the sum of all OUSD accounts
• Be at or below the value of the vault, so that it's fully backed
• Match the value passed in by the vault, so that yield calculations will be correct.

Because of rounding errors we basically can't have all three( outside of a 1/1e20? chance that the numbers for an update match). We have to pick our tradeoff.

The previous code matched the total supply to the sum of accounts, which because of rounding was almost always be higher than the passed in value. This gave us one property, sum matching, but at the cost of too much OUSD and yield calculations being slightly off. Suppose the vault looks at OUSD, sees a 10 dollar supply, records a $2 yield, and sets supply to $12. If the OUSD contract actually sets the supply to $13. The next time the vault checks for yield, it's going to undercount the yield by $1.

I've chosen to set the totalSupply to the exact value passed in. This ensures that all yield calculations are exactly correct. Because we are now rounding down rebasing ratios, this is always at or over the sum of all accounts, meaning all coins are fully backed. It does however mean that the sum of all OUSD account balances won't precisely match.

MAX_SUPPLY

Changing our rounding direction has fixed it so that we no longer go over MAX_SUPPLY.

The old code had an if statement intented to cap the max supply, regardless of what was passed in. I think this extra complexity is dangerous for two reasons:

• If our max supply is being set to over $340,282,366,920,938,463,463 in the next few years, it's overwhelmingly likely that we are under attack, rather than that our supply has legitimately grown that much. If we were under attack, throwing would be a far better solution than doing extra work to make the numbers fit and then continuing.
• Setting the OUSD value to different value than reported by the vault would throw off yield calculations.

I've chosen to simply throw if the new supply is over the max limit.

Gas

The old method made an unnecessary duplicate write to storage of _totalSupply.

Checks

Moved from two require's to five. This makes sure the invariants we need for correctness are written right into the code.

---

Contract change checklist:

• Code reviewed by 2 reviewers. See template and example.
• Unit tests pass
• Slither tests pass with no warning
• Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

[franckc]

Sorry for the super long turnaround time :( I finally had time to take a first pass at reviewing. Overall I like the simplicity and the addition of the invariants. I could not find any issue with these changes but I really need to spend more time thinking this thru... I'm keeping this on my TODO list of tasks to complete this week.

@tomlinton any feedback on your side?

[tomlinton]

Yep sorry also for the slow response. I've looked it through and it makes sense to me. I'm still mulling it over in the back of my head though.

[franckc]

Old PR. Closing.

[DanielVF]

From the amazing Rappie:

POC

  amount = ethers.BigNumber.from("1000000000");
  await usdt.approve(vault.address, amount);
  await vault.mint(usdt.address, amount, 0);

  amount = ethers.BigNumber.from("2000000000000000000000");
  await ousd.changeSupply(amount);

  await ousd.rebaseOptOut();

  // 3 seems to give some rounding errors
  amount = ethers.BigNumber.from("3");
  await vault.redeem(amount, 0);

  await ousd.rebaseOptIn(); // this reverts!

It seems this revert is because of small rounding errors.

OUSD.rebaseOptIn() nonRebasingSupply 1999999999999999995997
OUSD.rebaseOptIn() balanceOf(msg.sender) 1999999999999999995998

These rounding errors are harmless when looking at the amounts. In this case it has big side effects because of an underflow:

nonRebasingSupply = nonRebasingSupply.sub(balanceOf(msg.sender));

This should result in 0.

[sparrowDom]

@DanielVF since this PR has been created we have switched to 1e27 format of rebasingCreditsPerToken. Do you think the rounding down is still relevant?