Yield Delegation / Rebasing Token Rewrite

[sparrowDom]

We are revamping the our rebasing token contract.

The primary objective is to allow delegated yield. Delegated yield allows an account to seemlessly transfer all earned yield to another account.

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 18d4a51

[codecov]

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 17.97%. Comparing base (dc79701) to head (28d83f9).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
contracts/contracts/token/OUSD.sol	84.78%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2298       +/-   ##
===========================================
- Coverage   53.36%   17.97%   -35.40%     
===========================================
  Files          79       79               
  Lines        4098     4134       +36     
  Branches     1079     1087        +8     
===========================================
- Hits         2187      743     -1444     
- Misses       1908     3389     +1481     
+ Partials        3        2        -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DanielVF]

This is the actual chart of possible transfer rounding error vs rebasing credits per token.

In other words, it's only a problem at small values.

If we change to log view, which more accurately displays time moving along, we can see that the error is still packed on the far right side.

If we chop off some of the left side, we see that the shape of the curve stays the same, but the error goes way down.

In short the max error is basically 1e18 / rebasingCreditsPerToken.

When we create a new oToken, we send at least 1e16 tokens to a dead address. These tokens can no longer be spent, and represent 1e25 rebasing credits. When compared with the max supply allowed, these make for a minimum of 29387 rebasingCreditsPerToken possible, and a roughly 1/29387 max token error. If we bump up to assuming that people will lose access to at least one eth/usd, then that bumps up to roughly 1/1e7 max error, or less than a millionth of an ETH.

Harder guarantees

If we wanted harder guarantees, we either reduce the max totalSupply, or put a minimum on how far down the rebasingCreditsPerToken could go in a rebase. Doing either of these would result in yield that was not rebased to the users once the limit was hit.

if(rebasingCreditsPerToken>1e9){
    rebasingCreditsPerToken = 1e9;
}

[pandadefi]

A good practice is to keep the allowance at maximum value if it's set to max value, it saves some gas avoiding a write. You could use unchecked math

[pandadefi]

You could cache the allowance otherwise it's read on line 225 and on line 227.

[sparrowDom]

good point thanks: d285b32

[DanielVF]

After decoding the certora ping on this method, I've tracked it down to bad accounting created when account credits are > 0 but < rebasingCreditsPerToken. I'm not sure that this can actually happen in real life, however, if it does then the already rebasing credits on that are already account are not accounted for when updating the globals.

1. Do we need the balance==0 check at all?
2. If so, can we replace it with a creditBalances[_account] == 0 check, which should be a stronger check?

Me to certora.

Looks like the exampe is a rebasing account that is opting into being a rebasing account. It's skipping the accounting check because the balance is zero. However that zero balance in the example is coming from having some credits on the account that are divided by a larger global rebasingCreditsPerToken, resulting in 0.

I'm not sure this is a situation that could actually happen in practice, since in order to have any credits at all, then the account would have had to been transfered or minted >= 1 raw balance. If they had 1 raw balance, then they would have had account credits = the global rebasingCreditsPerToken. Since global rebasingCreditsPerToken only goes down, it shouldn't be possible in real life to have a rebasing account with (account credits < rebasingCreditsPerToken AND account credits > 0) and thus should not be possible to have an stdRebasing account with credits but not a balance.

Nevertheless, we'll probably the code so this rule would pass successfully as the rule is currently written. Given a choice, I'd rather have the code be correct in all circumstances, not just all possible circumstances.

[DanielVF]

Test reproduction:

ousd.mint(user, 1);
 // Negative rebase shouldn't happen in the real world.
ousd.changeSupply(ousd.totalSupply()-1e18);
// At this point the user has a 0 balance and nonzero credits.
vm.prank(user);
ousd.rebaseOptIn();

[DanielVF]

I just temporarily altered the check to use creditBalances[_account] == 0, so that we can see with certora if there are any other bad behaviors out of this function.

[sparrowDom]

Yes agreed, the creditBalance check should be a stronger one and still perform its old duty by allowing an empty rebasing account to explicitly optIn into rebasing.

I think your finding is correct that this failed state can only be reached if one rebases negatively consequently changing the supply downwards.

The storage data from the tests suggest the same with really large numbers for rebasingCreditsPerToken:

[DanielVF]

@sparrowDom What was the reason for allowing zero balance rebaseOptIns for rebasing accounts? Just ease of setting up new contracts?

[sparrowDom]

Yes that is the only reason I can think of. We do allow calling rebaseOptIn for EOA accounts with 0 balance as well, though I see no real utility there.