Investors claiming their maxDeposit by using the LiquidityPool.deposit() will cause that other users won't be able to claim their maxDeposit/maxMint

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L141-L144 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L427-L441

Vulnerability details

Impact

• Claiming deposits using the LiquidityPool.deposit() will cause the Escrow contract doesn't have enough shares to allow other investors to claim their maxDeposit or maxMint values for their deposited assets

Proof of Concept

• Before an investor can claim their deposits, they first needs to request the deposit and wait for the Centrigue Chain to validate it in the next epoch.

• Investors can request deposits at different epochs without the need to claim all the approved deposits before requesting a new deposit, in the end, the maxDeposit and maxMint values that the investor can claim will be increased accordingly based on all the request deposits that the investor makes.

• When the requestDeposit of the investor is processed in the Centrifuge chain, a number of TrancheShares will be minted based on the price at the moment when the request was processed and the total amount of deposited assets, this TrancheShares will be deposited to the Escrow contract, and the TrancheShares will be waiting for the investors to claim their deposits.

• When investors decide to claim their deposit they can use the LiquidityPool.deposit() function, this function receives as arguments the number of assets that are being claimed and the address of the account to claim the deposits for.

  function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
  shares = investmentManager.processDeposit(receiver, assets);
  emit Deposit(address(this), receiver, assets, shares);
  }

• The LiquidityPool.deposit() function calls the InvestmentManager::processDeposit() which will validate that the amount of assets being claimed doesn't exceed the investor's deposit limits, will compute the deposit price in the InvestmentManager::calculateDepositPrice(), which basically computes an average price for all the request deposits that have been accepted in the Centrifuge Chain, each of those request deposits could've been executed at a different price, so, this function, based on the values of maxDeposit and maxMint will estimate an average price for all the unclaimed deposits, later, using this computed price for the deposits will compute the equivalent of TrancheTokens for the CurrencyAmount being claimed, and finally, processDeposit() will transferFrom the escrow to the investor account the computed amount of TranchTokens.

  function processDeposit(address user, uint256 currencyAmount) public auth returns (uint256 trancheTokenAmount) {
  address liquidityPool = msg.sender;
  uint128 _currencyAmount = _toUint128(currencyAmount);
  require(
      //@audit-info => orderbook[][].maxDeposit is updated when the handleExecutedCollectInvest() was executed!
      //@audit-info => The orderbook keeps track of the number of TrancheToken shares that have been minted to the Escrow contract on the user's behalf!
      (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit && _currencyAmount != 0),
      "InvestmentManager/amount-exceeds-deposit-limits"
  );
  
  //@audit-info => computes an average price for all the request deposits that have been accepted in the Centrifuge Chain and haven't been claimed yet!
  uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
  require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");
  
  //@audit-info => Based on the computed depositPrice will compute the equivalent of TrancheTokens for the CurrencyAmount being claimed
  uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(_currencyAmount, liquidityPool, depositPrice);
  
  //@audit-info => transferFrom the escrow to the investor account the computed amount of TranchTokens.
  _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);
  trancheTokenAmount = uint256(_trancheTokenAmount);
  }

The problem occurs when an investor hasn't claimed their deposits and has requested multiple deposits on different epochs at different prices. The InvestmentManager::calculateDepositPrice() function will compute an equivalent/average price for all the requestDeposits that haven't been claimed yet. Because of the different prices that the request deposits where processed at, the computed price will compute the most accurate average of the deposit's price, but there is a slight rounding error that causes the computed value of trancheTokenAmount to be slightly different from what it should exactly be.

• That slight difference will make that the Escrow contract transfers slightly more shares to the investor claiming the deposits by using the LiquidityPool.deposit()
• As a result, when another investor tries to claim their maxDeposit or maxMint, now the Escrow contract doesn't have enough shares to make whole the request of the other investor, and as a consequence the other investor transaction will be reverted. That means the second investor won't be able to claim all the shares that it is entitled to claim because the Escrow contract doesn't have all those shares anymore.

Coded PoC

• I used the LiquidityPool.t.sol test file as the base file for this PoC, please add the below testPoC to the LiquidityPool.t.sol file

• In this PoC I demonstrate that Alice (A second investor) won't be able to claim her maxDeposit or maxMint amounts after the first investor uses the LiquidityPool.deposit() function to claim his maxDeposit() assets. The first investor makes two requestDeposit, each of them at a different epoch and at a different price, Alice on the other hand only does 1 requestDeposit in the second epoch.

• Run this PoC two times, check the comments on the last 4 lines, one time we want to test Alice claiming her deposits using LiquidityPool::deposit(), and the second time using LiquidityPool::mint()

  • The two executions should fail with the same problem.

    function testDepositAtDifferentPricesPoC(uint64 poolId, bytes16 trancheId, uint128 currencyId) public {
        vm.assume(currencyId > 0);

        uint8 TRANCHE_TOKEN_DECIMALS = 18; // Like DAI
        uint8 INVESTMENT_CURRENCY_DECIMALS = 6; // 6, like USDC

        ERC20 currency = _newErc20("Currency", "CR", INVESTMENT_CURRENCY_DECIMALS);
        address lPool_ =
            deployLiquidityPool(poolId, TRANCHE_TOKEN_DECIMALS, "", "", trancheId, currencyId, address(currency));
        LiquidityPool lPool = LiquidityPool(lPool_);
        homePools.updateTrancheTokenPrice(poolId, trancheId, currencyId, 1000000000000000000);

        //@audit-info => Add Alice as a Member
        address alice = address(0x23232323);
        homePools.updateMember(poolId, trancheId, alice, type(uint64).max);

        // invest
        uint256 investmentAmount = 100000000; // 100 * 10**6
        homePools.updateMember(poolId, trancheId, self, type(uint64).max);
        currency.approve(address(investmentManager), investmentAmount);
        currency.mint(self, investmentAmount);
        lPool.requestDeposit(investmentAmount, self);

        // trigger executed collectInvest at a price of 1.25
        uint128 _currencyId = poolManager.currencyAddressToId(address(currency)); // retrieve currencyId
        uint128 currencyPayout = 100000000; // 100 * 10**6                                          
        uint128 firstTrancheTokenPayout = 80000000000000000000; // 100 * 10**18 / 1.25, rounded down
        homePools.isExecutedCollectInvest(
            poolId, trancheId, bytes32(bytes20(self)), _currencyId, currencyPayout, firstTrancheTokenPayout
        );

        // assert deposit & mint values adjusted
        assertEq(lPool.maxDeposit(self), currencyPayout);
        assertEq(lPool.maxMint(self), firstTrancheTokenPayout);

        // deposit price should be ~1.25*10**18 === 1250000000000000000
        assertEq(investmentManager.calculateDepositPrice(self, address(lPool)), 1250000000000000000);

        // second investment in a different epoch => different price
        currency.approve(address(investmentManager), investmentAmount);
        currency.mint(self, investmentAmount);
        lPool.requestDeposit(investmentAmount, self);

        // trigger executed collectInvest at a price of 2
        currencyPayout = 100000000; // 100 * 10**6
        uint128 secondTrancheTokenPayout = 50000000000000000000; // 100 * 10**18 / 1.4, rounded down
        homePools.isExecutedCollectInvest(
            poolId, trancheId, bytes32(bytes20(self)), _currencyId, currencyPayout, secondTrancheTokenPayout
        );

        // Alice invests the same amount as the other investor in the second epoch - Price is at 2
        currency.mint(alice, investmentAmount);

        vm.startPrank(alice);
        currency.approve(address(investmentManager), investmentAmount);
        lPool.requestDeposit(investmentAmount, alice);
        vm.stopPrank();

        homePools.isExecutedCollectInvest(
            poolId, trancheId, bytes32(bytes20(alice)), _currencyId, currencyPayout, secondTrancheTokenPayout
        );

        uint128 AliceTrancheTokenPayout = 50000000000000000000; // 100 * 10**18 / 1.4, rounded down

        //@audit-info => At this point, the Escrow contract should have the firstTrancheTokenPayout + secondTrancheTokenPayout + AliceTrancheTokenPayout
        assertEq(lPool.balanceOf(address(escrow)),firstTrancheTokenPayout + secondTrancheTokenPayout + AliceTrancheTokenPayout);

        // Investor collects his the deposited assets using the LiquidityPool::deposit()
        lPool.deposit(lPool.maxDeposit(self), self);

        // Alice tries to collect her deposited assets and gets her transactions reverted because the Escrow doesn't have the required TokenShares for Alice!
        vm.startPrank(alice);

        //@audit-info => Run the PoC one time to test Alice trying to claim their deposit using LiquidityPool.deposit()
        lPool.deposit(lPool.maxDeposit(alice), alice);

        //@audit-info => Run the PoC a second time, but now using LiquidityPool.mint()
        // lPool.mint(lPool.maxMint(alice), alice);
        vm.stopPrank();
    }

Tools Used

Manual Audit

Recommended Mitigation Steps

• I'd recommend to add a check to the computed value of the _trancheTokenAmount in the InvestmentManager::processDeposit(), if the _trancheTokenAmount exceeds the maxMint() of the user, update it and set it to be the maxMint(), in this way, the rounding differences will be discarded before doing the actual transfer of shares from the Escrow to the user, and this will prevent the Escrow from not having all the required TranchToken for the other investors

  
  function processDeposit(address user, uint256 currencyAmount) public auth returns (uint256 trancheTokenAmount) {
  address liquidityPool = msg.sender;
  uint128 _currencyAmount = _toUint128(currencyAmount);
  require(
      (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit && _currencyAmount != 0),
      "InvestmentManager/amount-exceeds-deposit-limits"
  );
  
  uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
  require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");
  
  uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(_currencyAmount, liquidityPool, depositPrice);
  
  //@audit => Add this check to prevent any rounding errors from causing problems when transfering shares from the Escrow to the Investor!

• if (_trancheTokenAmount > orderbook[user][liquidityPool].maxMint) _trancheTokenAmount = orderbook[user][liquidityPool].maxMint;

  _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user); trancheTokenAmount = uint256(_trancheTokenAmount); }

• After applying the suggested recommendation, you can use the provided PoC on this report to verify that the problem has been solved.

Assessed type

Math

[raymondfam]

Accounting mess up indeed.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as remove high or low quality report

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx (sponsor) confirmed

[gzeon-c4]

https://github.com/code-423n4/2023-09-centrifuge-findings/issues/210#issuecomment-1728512207

[gzeon-c4]

This issue described a protocol specific issue with multiple deposit/withdrawal where #34 is a generic 4626 rounding issue, and therefore not marked as duplicate of #34. In terms of severity, this does not directly lead to a loss of fund but will affect the availability of the protocol, hence Med.

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[hieronx]

Mitigated in https://github.com/centrifuge/liquidity-pools/pull/166