Redacted Cartel contest - gogo's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L60-L78 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L80-L97

Vulnerability details

[H] Price manipulation can lead to users lossing funds

Impact

Early malicious user will profit from future users' deposits while future users' will loose funds/value.

Proof of Concept

An early user can call the deposit function of any of the ERC4626 vaults with only 1 wei of the asset and respectively get 1 wei of the shares in return. After that the attacker may send 10000 * 10 ** 18 wei of the asset to inflate the share price from 1 to 1 * 10 ** 22 ((1 + 10000e18 - 1) / 1). The next user who deposits some amount of asset will receive significatly less shares - if the next user deposits 20000 * 10 ** 18 wei of the asset they will receive only 2 wei of shares which means they have lost half of their money if they redeem right after that.

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L60-L78
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L80-L97

Tools Used

manula review

Recommended Mitigation Steps

Require a minimum amount of initial deposit and burn a portion of the initial shares.

    function deposit(uint256 assets, address receiver)
        public
        virtual
        returns (uint256 shares)
    {
        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
        
        if (totalAssets() == 0) {
            _mint(address(0), initialShares);
        }

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

        _mint(receiver, shares);

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

        afterDeposit(receiver, assets, shares);
    }

    function mint(uint256 shares, address receiver)
        public
        virtual
        returns (uint256 assets)
    {
        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

        assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
        
        if (totalAssets() == 0) {
          _mint(address(0), initialShares);
        }

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

        _mint(receiver, shares);

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

        afterDeposit(receiver, assets, shares);
    }

Reference

Reference to how another protocol has fixed this same issue:
https://github.com/sentimentxyz/protocol/pull/232/commits/16557a3e82dce3c6a03d5e8f7280e1af12fe3bf0