transmissions11/solmate

[ERC4626]: implement totalAssets in base class

jaglinux opened this issue · 3 comments

jaglinux commented

https://github.com/Rari-Capital/solmate/blob/eaaccf88ac5290299884437e1aee098a96583d54/src/mixins/ERC4626.sol#L122
Trying to understand why there is no implementation for totalAssets() ?
totalAssets() should return vault's balance of underlying asset. The derived contract or the implementer could override the logic if required.

transmissions11 commented

using balanceOf the asset is fairly dangerous cuz ur susceptible to vault donation attacks, if we were to provide another state variable to track the vaults total assets we wouldnt know what uint size to make it so it can pack with the user's other state. for those reasons i'd prefer to leave this unimplemented

  • 👍1
frangio commented

susceptible to vault donation attacks

Can you explain the potential attack further? Are there concrete circumstances under which a donation could be used as part of an attack?

  • 👍1
transmissions11 commented

Can you explain the potential attack further? Are there concrete circumstances under which a donation could be used as part of an attack?

the attack wouldn't be against the vault itself (it should be secure in that case) but any protocols which depend on the vault's share price not skyrocketing randomly. the ability to donate and boost the share price dramatically can really screw up the economics of different defi projects (notably lending markets)

an example of the attack in the wild is the cream yearn hack: https://mudit.blog/cream-hack-analysis/