ERC4626 does not work with fee-on-transfer tokens

[code423n4]

Lines of code

https://github.com/Rari-Capital/solmate/blob/8c0e278900fe552fa0739975bde21c6a07d84ccf/src/mixins/ERC4626.sol#L52

Vulnerability details

Impact

The docs/video say ERC4626.sol is in scope as its part of TurboSafe

The ERC4626.deposit/mint functions do not work well with fee-on-transfer tokens as the amount variable is the pre-fee amount, including the fee, whereas the totalAssets do not include the fee anymore.

This can be abused to mint more shares than desired.

function deposit(uint256 amount, address to) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(amount)) != 0, "ZERO_SHARES");

    // Need to transfer before minting or ERC777s could reenter.
    asset.safeTransferFrom(msg.sender, address(this), amount);

    _mint(to, shares);

    emit Deposit(msg.sender, to, amount, shares);

    afterDeposit(amount, shares);
}

POC

A deposit(1000) should result in the same shares as two deposits of deposit(500) but it does not because amount is the pre-fee amount. Assume a fee-on-transfer of 20%. Assume current totalAmount = 1000, totalShares = 1000 for simplicity.

• deposit(1000) = 1000 / totalAmount * totalShares = 1000 shares
• deposit(500) = 500 / totalAmount * totalShares = 500 shares. Now the totalShares increased by 500 but the totalAssets only increased by (100% - 20%) * 500 = 400. Therefore, the second deposit(500) = 500 / (totalAmount + 400) * (newTotalShares) = 500 / (1400) * 1500 = 535.714285714 shares.

In total, the two deposits lead to 35 more shares than a single deposit of the sum of the deposits.

Recommended Mitigation Steps

amount should be the amount excluding the fee, i.e., the amount the contract actually received. This can be done by subtracting the pre-contract balance from the post-contract balance. However, this would create another issue with ERC777 tokens.

Maybe previewDeposit should be overwritten by vaults supporting fee-on-transfer tokens to predict the post-fee amount. And do the shares computation on that, but then the afterDeposit is still called with the original amount and implementers need to be aware of this.

[Joeysantoro]

This is intended. Fee-on-transfer functions can be implemented in another base contract. this contract is only one implementation of the standard.

[GalloDaSballo]

Given no context I would side with the sponsor as the ERC4626 standard is meant to work with ERC20 Standard tokens.

However, in bringing the ERC4626 mixin into scope, the sponsor's code has an explicit mention of ERC777 which does open up to the possibility of having fees on transfer.

Because ultimately the warden didn't stretch the scope (as that happened because of the mixin), and the warden showed a way to provide leakage of value or denial of service exclusively if the mixin code is used in conjunction with a feeOnTransfer token.

Given that the mixin code comments explicitly mention attempting to support non-standard tokens.

I believe medium severity to be valid as any type of misbehavior is contingent on deploying an ERC4626 mixin with a feeOnTransfer Token.

Given the circumstances, the best recommendation for developers is to use the mixin with ERC20 Standard Tokens.