ERC4626

[Amxx]

Fixes #3393

Reference EIP.

We still have not decided the scope of customization we want to include in this implementation (fees?)
Should ERC165 be included? The ERC does not mention it.

Feel free to discuss/ask for features.

PR Checklist

• Tests
• Documentation
• Changelog entry
• Check ERC compliance

[DanielVF]

We have a rebasing currency, one thing we've done to checkup on exchange's support for it is to create a pair of accounts with matching amounts of our currency, one on-chain, and one on the exchange, then watch the balance changes between them and totals over time.

I've taken a similar approach to testing the both the solmate and this OZ ERC4626 implementation as a wrapper to our rebasing currency. Using a mainnet fork, I've fired up several pairs of accounts - one account of each pair never touches the wrapper, and one use the wrapper. Then at each step, the designated one of random twin pair does a random deposit or withdraw to the wrapper. Then after each step, the underlying currency rebases up (sometimes by up to 5x).

Looks like both solmate and OZ ERC4626 handle this well. There's a small drift between twins from the rounding, but it seems to be the correct size considering the number of transactions, and the overall 10,000,000x'ing of the base supply over the course of the testing. This rounding dust flows nicely to the control account that is just holding the wrapped currency without transacting.

This doesn't really test anything beyond the deposit/withdraw math, but it's comforting to see

[frangio]

I'm thinking we should add fees. Isn't the contract otherwise vulnerable to sandwich attacks of the transaction where interest is accrued? I feel like I must be missing something there. Maybe the interest would just be small enough that a sandwich attack wouldn't really be profitable.

In any case projects might want fees so I feel it's something we should have by default (with a default of 0 fees like the flash mint) without requiring further customization.

[Amxx]

It's true that in case the interest is released at once and not using a flash bot, then a sandwich attack can drain the interests. We should document best practices, in particular, if the interest is per block (like vesting) then this attack is not possible.

If we have a clear way of formulating fees, then I would agree we can add it, but it not clear to me that the fees are that simple to set:

• fees on buy? fees on sell? both?
• percentage fees? flat fees?

AFAIK, the fees are as simple as "override the previewXxx" function.

[frangio]

It's not so simple because you need to override the two pairs of functions.

[robercano]

It seems to me that it would be enough to have two internal calculateBuyFee and calculateSellFee that receive the amount being bought or sold. Then integrators could override this to provide the required fee scheme.

…

On Sat, 21 May 2022, 18:14 Francisco Giordano, ***@***.***> wrote: It's not so simple because you need to override the two pairs of functions. — Reply to this email directly, view it on GitHub <#3171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA24ZL25UAH3ISIDSUSIAFTVLEDUDANCNFSM5NX2BYQA> . You are receiving this because you commented.Message ID: ***@***.***>

[frangio]

On the question of fees we've decided to release initially without fees. They may be added later.

[frangio]

It's a little confusing that this hardcodes 12 and 18 decimals, given that there's also a test where the token has 18 decimals. I think these helpers should be defined inside a describe block where we use these decimals.

[Amxx]

they are used everywhere expect for the multiple mint, deposit, redeem & withdrawal test.

[frangio]

I wanted to analyze the conversion functions to make sure I understood exactly the way we're pricing shares in the degenerate cases.

Our conversion functions are not symmetrical, they differ depending on whether the input is a number of assets or shares (e.g. mint vs deposit). This is our currently implemented behavior:

A ↗ S	Z	NZ
Z	1	!
NZ	1	%

A ↙ S	Z	NZ
Z	1	0
NZ	1	%

(Sorry for the symbols but I liked the succinct presentation. A: totalAssets, S: totalSupply, A→S: convert to shares, A←S: convert to assets, Z: zero, NZ: non-zero, 1: decimals-adjusted 1-to-1 price, %: price based on vault ratio, !: revert)

Note that they differ in the case that there are zero assets and non-zero shares. I don't think this is right, I think we should aim for symmetry. I would suggest reverting independently of the conversion direction.

[Amxx]

I don't think there is any inconsistency here.

If we have 0 assets, and >0 shares, then:

• shares are worth 0 assets
• assets are worth an infinite amount of shares.

So "!" and "0" are in fact the value that corresponds to "%" in that particular case.

[frangio]

Mathematically % would be ∞ but that is not equivalent to a revert. Still think the asymmetry is a problem. Will try to implement the symmetrical behavior to revert in both cases without too much added overhead.

[Amxx]

This last commit adds quite a lot of sloads:

• totalSupply() can be expected to be dirty, so its only 100 gas each time
• totalAssets() can also be expected to be dirty, but includes a staticcall to another contract, which is not free.

It also checks totalAssets() in cases where totalSupply() is 0, which used to not be necessary (in that case the sload is cold).
All that to "fix" the behavior in a case where the vault is broken.

This case should never happen unless the vault is overridden in a way to enables slashing or loss of assets. I'm not sure we should include that by default. IMO this is something that should be added only if the vault mechanics are changed in a way that can cause it to be broken. We should not include it by default. Other devs should include it if their instance requires these additional checks

[frangio]

Ok, we're good.