w42d3n - ERC4626 does not work with fee-on-transfer tokens

[sherlock-admin]

w42d3n

unlabeled

ERC4626 does not work with fee-on-transfer tokens

Summary

ERC4626 does not work with fee-on-transfer tokens

Vulnerability Detail

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

Impact

This can be abused to mint more shares than desired.

Code Snippet

https://github.com/sherlock-audit/2022-10-astaria/blob/main/lib/astaria-gpl/src/ERC4626-Cloned.sol#L305-L322

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

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

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

Tool used

Visual inspection Manual Review

PoC

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

deposit(1000) = 1000 / totalSupply totalAssets = 1000 shares deposit(500) = 500 / totalSupply totalAssets = 500 shares. Now the totalAssets increased by 500 but the totalAssets only increased by (100% - 20%) 500 = 400. Therefore, the second deposit(500) = 500 / (totalSupply + 400) (newTotalAssets) = 500 / (1400) * 1500 = 535.714285714 assets. In total, the two deposits lead to 35 more assets than a single deposit of the sum of the deposits.

Recommendation

assets 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.

Another recommendation is to use the mixin with ERC20 Standard Tokens only by creating a whitelist.

Duplicate of #3

[w42d3n]

Escalate for 1 USDC: 'Duplicate of https://github.com/sherlock-audit/2022-10-astaria-judging/issues/3' [androolloyd} commented: 'we will not be supporting fee on transfer tokens at the time' I don't think this was mentioned in the documentation, therefore this funding should be valid. Or they will not be supporting fee on transfer tokens at the time due to this issue? In any case this finding and duplicates should be validated and rewarded imho.

[sherlock-admin]

Escalate for 1 USDC: 'Duplicate of https://github.com/sherlock-audit/2022-10-astaria-judging/issues/3' [androolloyd} commented: 'we will not be supporting fee on transfer tokens at the time' I don't think this was mentioned in the documentation, therefore this funding should be valid. Or they will not be supporting fee on transfer tokens at the time due to this issue? In any case this finding and duplicates should be validated and rewarded imho.

You've created a valid escalation for 1 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation accepted

[sherlock-admin]

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.