Popcorn contest - Blockian's results

Lines of code

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

Vulnerability details

A malicious early user/attacker can manipulate the Vault's price per share to take an unfair share of future users' deposits

Vulnerable Contract Vault.sol

Summary

A well-known attack vector for almost all shares based contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

Vulnerability Detail

An early malicious user can call vault.deposit(1) with 1 wei of asset token as the first depositor of the Vault, and get 1 share.

Then the attacker can adapter.deposit(10000e18 - 1, vaultAddress) with 10000e18 - 1 of asset tokens straight to the Adapter and inflate the price per share (since no new shares will be minted by the vault but the adapter will increase the vault balance) from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) since the formula is totalAssets() / supply (check #2 in code snippet) where totalAssets considers the vault balance in regard to the adapter (#3)).

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

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

(In addition, any user who deposits less than 10000e18 will receive no shares at all)

Impact

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker. The attacker will also be able to monitor the deployment of new vaults to replay this attack in any new vault created

Code Snippet

Vault deposit:

    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);
    }

Adapter deposit:

    function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        if (assets > maxDeposit(receiver)) revert MaxError(assets);

        uint256 shares = _previewDeposit(assets);
        _deposit(_msgSender(), receiver, assets, shares);

        return shares;
    }

    function _deposit(
        address caller,
        address receiver,
        uint256 assets,
        uint256 shares
    ) internal nonReentrant virtual override {
        IERC20(asset()).safeTransferFrom(caller, address(this), assets);
        
        uint256 underlyingBalance_ = _underlyingBalance();
        _protocolDeposit(assets, shares);
        // Update the underlying balance to prevent inflation attacks
        underlyingBalance += _underlyingBalance() - underlyingBalance_;

        _mint(receiver, shares);

        harvest();

        emit Deposit(caller, receiver, assets, shares);
    }

Convert to shares and to assets calculations:

    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); // #1
    }

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

        return
            supply == 0
                ? shares
                : shares.mulDiv(totalAssets(), supply, Math.Rounding.Down); // #2
    }

Total Assets function - checks the vaults balance in the adapter:

    function totalAssets() public view returns (uint256) {
        return adapter.convertToAssets(adapter.balanceOf(address(this))); // #3
    }

Tools Used

Manual review

Recommendation

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a part of the initial mints as a reserve to the Owner (feeRecipient, address(0) or any trustworthy candidate) so that the price per share can be more resistant to manipulation.