ERC4626: Vault: Implementation for totalAssets() in base contract

Support Contracts
1

I had similar discussion on totalAssets() in another vault implementation, bringing up the same here.

 function totalAssets() public view virtual override returns (uint256) {
        return _asset.balanceOf(address(this));
    }

Anyone can transfer asset to this contract from outside (vault donation attack) and disrupt the ratio.
Hence a new variable is required to track the assets deposited only through this contract. The implementer can use desired uint size to pack variables with other state variables.
So its better to have no implementation for totalAssets() and let the derived class / implementer can code their own logic.

2

Thanks for bringing this up @Jagadish_K.

Can you explain in what scenario a donation can be turned into an attack?

Can you provide a link the similar discussion that you mention?

3

Please refer to comments by t11 in https://github.com/Rari-Capital/solmate/issues/266 and I agree with his reasoning.
As per the spec,https://eips.ethereum.org/EIPS/eip-4626#methods

totalAssets :Total amount of the underlying asset that is “managed” by Vault.

So the contract needs to track the assets which is deposited only through this contract.
Regarding the attack. I am not sure if the attacker can benefit but can alter the ratios in functions which uses totalAssets()

4

If the underlying asset is donated, it becomes managed by the vault, in the sense that it will be withdrawable by redeeming shares.

My question is: what are the concrete risks here? What can go wrong in a "donation attack"?

5

The default implementation is designed in a way that all assets owned by the vault are in fact managed by the contract. In particular, sending tokens directly to the vault is a way to "airdrop" assets to all the share holders, thanks to the fact that there assets are correctly managed by the vault.

I agree that in case the vault is overridden, then changing the way totalAssets() work could be essential, but IMO that doesn't justify leaving it unimplemented.

6

I can think of scenario where user B who deposits asset after the "donation" receives less number of shares compared to "non donation" scenario.
User A deposits 50 assets and receives 50 shares.
Donation received by vault : 50 assets.
User B deposits 50 assets and receives 25 shares only (instead of 50)

User A can now withdraw 100 assets for 50 shares whereas User B can withdraw 50 assets for 25 shares. This benefits user A (but no loss to user B, but no gains either).
Only question is if the donation is approved by vault or not ? If not , then the rewards are skewed.

7

I would prefer to have a state variable to track assets in the vault and a donate / add asset function in derived or implementer class to track the rewarded assets.
For airdrop scenarios, the assets in vaults are automatically added (cannot prevent the transfer anyways) and the vault manager can decide whether to increment the state variable or not.

8

I honestly don't see that as an attack:

  • B can redeem everything he deposited. There is no loss of funds for B.
  • Its true that all the donation goes to A, but again A was the only one to have share in the vault at the moment of the donation.

Basically the donation is split between share holders, proportionally to how many shares they have.

It might sound cliché, but I would argue me what you describe as a bug is in fact a feature !

1 Like
9

Also, its not possible to prevent "donations" from happening. You would rather have these funds locked forever than redistributed by default ?

Some devs may want to track "the managed assets" in a variable (and I believe it is very easy to do so with some overrides), but I don't think it should be the default. It requires more storage, makes many operations more expensive, and can result in loss of funds.

I would also rather have a default that is coherent, is well documented, correspond to some usecase, and that can easily be overridden ... than having no default.

10

This is not the only alternative... If deposits are tracked in a variable, surplus donated funds can be somehow recoverable.

11

This requires an owner or admin.

It is definitely feasible, maybe some people would find that desirable, but IMO it should not be seen as the "right" way.

12

As pointed out previously, the spec says that totalAsset() is the

Total amount of the underlying asset that is “managed” by Vault.

The question is,

should the default be that all assets held by the vault are "managed" by the vault?

I very strongly feel that yes, and that if some assets are to be excluded from the "managed" bag, it requires a special treatment. We should ensure it is as easy as possible to implement this special treatment, but we should not think its the default one.

13

The question is,

should the default be that all assets held by the vault are "managed" by the vault?

This is a valid question, maybe needs to be clarified by EIP 4626 authors ?
At-least there should be note in the spec so that users are aware of the non default scenario.

14

Unfortunately, a lot of things are unclear in the ERC. Sometimes I wonder why ERC take so much time to get finalized, but this one is the perfect example of one that was IMO finalized to fast.

1 Like
15

Hi we are currently integrating the OZ contracts and are looking for a solution to the "donation attack".

This is a real attack where innocent users can lose part of their deposits:

  • Alice (the attacker) deposits 1 wei assets and receives 1 share
  • Alice now sends 1e18 tokens to the contract. At this point, 1 share can be redeemed for 1e18+1 tokens.
  • Bob sends 1.5e18 tokens to the contract. He receives 1 share.
  • So there now are 2 shares for 2.5e18 tokens. Alice can now redeem her share for 1.25e18 tokens, effectively she has taken 0.25e18 tokens from Bob's deposit
16

So the problem is basically that the Alice can inflate the value of a share in such a way that rounding errors become significantly large.

There is also a kind of standard solution for the problem, which is to start with a minimal amount of shares (e.g. 1000)

17

@Jelle_Gerbrandy Sorry we missed this earlier. We've actually been actively discussing mitigations for this issue over the past week. Please see the discussion here:

This is discussed in the issue as one of the options. However, my preferred solution right now is to directly detect losses due to rounding errors and add a hard limit to that, reverting a deposit if it passes the threshold.

Please do participate in the issue!