Popcorn contest - Josiah's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L294-L301

Vulnerability details

Impact

As also encountered by Uniswap V2 and other protocols, the first depositor of an ERC4626 vault can maliciously manipulate the share price by depositing as low as 1 wei of liquidity prior to deliberately inflating ERC4626.totalAssets to as high as 1:1e18.

This malicious act forces all subsequent deposits to be directed by this share price as a base. Moreover, due to Math.Rounding.Down, if this malicious initial deposit managed to front-run someone else depositing, this depositor could end up receiving 0 shares and losing his deposited assets to make things even worse.

As delineated in the code block below, shares are calculated in the return statement, i.e. supply == 0 ? assets : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down). In case of a very high share price, due to totalAssets() > assets * supply, shares will be returned 0 due to this precision issue.

Vault.sol#L294-L301

    function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

        return
            supply == 0
                ? assets
                : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    }

Proof of Concept

Here is the exploit scenario:

A malicious early user can call deposit() with 1 wei of asset token as the first depositor, and gets 1 wei of shares as is evidenced in the ternary return statement above.

Next, the attacker will send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1).

Consequently, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

He/she will immediately lose 9999e18 or equivalently half of the deposits if redeem() is called right after deposit().

Conclusion: The attacker can profit from future users' deposits whilst the late users will lose part of their funds to the attacker.

Recommended Mitigation Steps

It is recommended sending the first 1000 shares to address 0, a mitigation approach adopted by the Uniswap V2 protocol.

Additionally, the protocol should also look into introducing and incorporating slippage protections to better prevent the malicious act from transpiring.

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.