FEE ON TRANSFER TOKENS UNSUPPORTED IN VAULT.SOL

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L134-L158 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L170-L198 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L253-L278 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L211-L240

Vulnerability details

Impact

Some ERC20 tokens, e.g. STA, PAXG, incur a transfer fee whereas some like USDT, USDC do not currently charge a fee but may do so in the future.

In the current implementation of Vault.sol, it is assumed that the received amount is the same as the assets transfer amount when adding tokens via deposit() and mint(). However, due to how fee-on-transfer tokens work, much less will be received than what was transferred. Consequently, later users may not be able to successfully redeem/withdraw their shares, as it may revert due to insufficient contract balance.

Proof of Concept

Here is a typical Fee-on-transfer scenario:

1. Contract calls transfer from contractA 100 tokens to current contract
2. Current contract thinks it has received 100 tokens
3. It updates balances to increase +100 tokens
4. In actuality, contract received only 90 tokens

This breaks the whole math for this given token and mess up with future accounting.

Vault.sol#L134-L158

    function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
        );

        shares = convertToShares(assets) - feeShares;

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        _mint(receiver, shares);

        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));

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

Vault.sol#L253-L278

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {
        if (receiver == address(0)) revert InvalidReceiver();

        if (msg.sender != owner)
            _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

        uint256 feeShares = shares.mulDiv(
            uint256(fees.withdrawal),
            1e18,
            Math.Rounding.Down
        );

        assets = convertToAssets(shares - feeShares);

        _burn(owner, shares);

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        adapter.withdraw(assets, receiver, address(this));

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

Recommended Mitigation Steps

It is recommended:

1. comparing before and after the contract balance to get the actual transferred amount, or
2. disallowing tokens with deflationary nature to be added/deposited as tokens.

[c4-judge]

dmvt marked the issue as duplicate of #44

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory