Popcorn contest - savi0ur's results

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#L294-L301 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#L308-L315

Vulnerability details

Impact

The attacker can profit from future user's deposits. While the late users will lose part of their funds to the attacker.

Proof of Concept

A malicious early user can deposit() with 1 wei of asset token as the first depositor, and get 1 wei of shares.

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

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

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#L294-L301 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#L308-L315

Then the attacker can 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) .

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add a step to your deployment flow which will deposits some initial liquidity of the underlying token to the vault. That way the amount the attacker will need to ERC20.transfer to the system will be at least X * 1e18 instead of X which is unrealistic. This approach has been used long enough by various projects, for example in Uniswap V2

Anyone able to call changeFees function from Vault multiple times after block.timestamp < proposedFeeTime + quitPeriod satisfied, consider updating proposedFeeTime to some large value at the end of a changeFees call.

function changeFees() external {
    if (block.timestamp < proposedFeeTime + quitPeriod)
        revert NotPassedQuitPeriod(quitPeriod);

    emit ChangedFees(fees, proposedFees);
    fees = proposedFees;
}

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