Centrifuge - 0xklh's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L135-L137 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L160-L162 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L170-L172 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L192-L194 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L176-L184 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L148-L152

Vulnerability details

Impact

Protocols integrating with Centrifuge may wrongly assume that the pools are EIP-4626 compliant, possibly leading to various issues.

Proof of Concept

Preview functions

The following functions do not conform to the EIP-4626 specification: previewDeposit previewMint previewWithdraw previewRedeem

Specifically, the standard states (below for previewDeposit) :

*MUST NOT account for deposit limits like those returned from maxDeposit and should always act as though the deposit would be accepted, regardless if the user has enough tokens approved, etc.* 

Thus these functions were meant to be agnostic of the user. The implementations, however, do call the InvestmentManager with msg.sender as a parameter,

    /// @return shares that any user would get for an amount of assets provided
    function previewDeposit(uint256 assets) public view returns (uint256 shares) {
        shares = investmentManager.previewDeposit(msg.sender, address(this), assets);
    }

in order to compute the result based on the user's orderbook entry. This way previews only reflect already executed orders; if a user has not yet requested such order (or had it executed), the preview will return 0, as demonstrated by the following addition to the existing LiquidityPool.t.sol:

diff --git a/./LiquidityPool.t.sol.orig b/./LiquidityPool.t.sol
index 4e60ec1..b70a296 100644
--- a/./LiquidityPool.t.sol.orig
+++ b/./LiquidityPool.t.sol
@@ -1346,4 +1346,51 @@ contract LiquidityPoolTest is TestSetup {
         );
         investor.deposit(_lPool, amount, _investor); // deposit the amount
     }
+
+    function testPreviewDepositCompliance() public {
+        uint128 currencyId = 1;
+        uint64 poolId = 1;
+        bytes16 trancheId = bytes16("1");
+
+        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);
+
+        // 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 of the first 50% at a price of 1.2
+        uint128 _currencyId = poolManager.currencyAddressToId(address(currency)); // retrieve currencyId
+        uint128 currencyPayout = 50000000; // 50 * 10**6
+        uint128 firstTrancheTokenPayout = 41666666666666666666; // 50 * 10**18 / 1.2, rounded down
+        homePools.isExecutedCollectInvest(
+            poolId, trancheId, bytes32(bytes20(self)), _currencyId, currencyPayout, firstTrancheTokenPayout
+        );
+
+        // stack too deep
+        delete poolId;
+        delete trancheId;
+
+        // preview as a user with executed order
+        uint256 s0 = lPool.previewDeposit(investmentAmount);
+
+        // preview as a random user
+        vm.startPrank(address(1234));
+        uint256 s1 = lPool.previewDeposit(investmentAmount);
+        vm.stopPrank();
+
+        console.log("s0 : ", s0);
+        console.log("s1 : ", s1);
+        require(s0 == s1, "not compliant");
+    }
 }

All preview functions are constructed in the same manner as previewDeposit.

Rounding

This is a security consideration from the EIP-4626 standard:

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

Thus Mint, Withdraw, and their respective preview functions should round up.

Mint functions however call _calculateCurrencyAmount, that rounds down at line 614:

    function _calculateCurrencyAmount(uint128 trancheTokenAmount, address liquidityPool, uint256 price)
        internal
        view
        returns (uint128 currencyAmount)
    {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);

        uint256 currencyAmountInPriceDecimals = _toPriceDecimals(
            trancheTokenAmount, trancheTokenDecimals, liquidityPool
        ).mulDiv(price, 10 ** PRICE_DECIMALS, MathLib.Rounding.Down);

        currencyAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, currencyDecimals, liquidityPool);
    }

When Withdraw functions call _calculateTrancheTokenAmount, rounding down at line 599:

    function _calculateTrancheTokenAmount(uint128 currencyAmount, address liquidityPool, uint256 price)
        internal
        view
        returns (uint128 trancheTokenAmount)
    {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);

        uint256 currencyAmountInPriceDecimals = _toPriceDecimals(currencyAmount, currencyDecimals, liquidityPool).mulDiv(
            10 ** PRICE_DECIMALS, price, MathLib.Rounding.Down
        );

        trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
    }

Other considerations

• One could argue that the need to post and have executed an order to Centrifuge before deposit/mint would break the requirement to support the EIP-20 approve/transferFrom on asset as a deposit/mint flow.

• I could not assess wether a partial freeze of the pool during an epoch (that is, a ban on junior redemption and senior investments to address a breach of subordination ratio) would break the following requirement, common to the max* functions (below for maxMint):

MUST return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset.

• Although my opinion might differ, I have considered these previous C4 rulings related to similar issues (including rounding) when setting the severity to High: https://github.com/code-423n4/2022-06-notional-coop-findings/issues/155 https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47

Tools Used

Manual review

Recommended Mitigation Steps

Ensure rounding is compliant with EIP-4626. Consider changing the way outputs of preview functions are calculated.

Assessed type

ERC4626