Centrifuge - 0xPacelli's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L598-L600 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L612-L614

Vulnerability details

Impact

LiquidityPool.sol is tokenized vault expected to be ERC-4626 compliant. Certain view methods from the vault call methods on InvestmentManager to read values but the methods on this contract does not handle rounding according to the standard.

Other protocols acting as investors that integrate with Centrifuge might wrongly assume that functions in the protocol handle rounding as per ERC-4626 specifications. For this reason I evaluate this vulnerability to MEDIUM.

Summary

The ERC-4626 standard describe certain security considerations for the view functions regarding rounding:

Finally, EIP-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:

1. 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.

2. 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.

It is the second point that is not follow in InvestmentManager::_calculateTrancheTokenAmount according to the standard.

Proof of Concept

LiquidityPool::previewWithdraw can be used by investors to know how much shares they need to redeem in order to receive an amount of assets from the pool.

This function calls InvestmentManager::previewWithdraw, and here the rounding issues appear.

    /// @return trancheTokenAmount is type of uin256 to support the EIP4626 Liquidity Pool interface
    function previewWithdraw(address user, address liquidityPool, uint256 _currencyAmount)
        public
        view
        returns (uint256 trancheTokenAmount)
    {
        uint128 currencyAmount = _toUint128(_currencyAmount);
        uint256 redeemPrice = calculateRedeemPrice(user, liquidityPool);
        if (redeemPrice == 0) return 0;

        trancheTokenAmount = uint256(_calculateTrancheTokenAmount(currencyAmount, liquidityPool, redeemPrice));

The function then calls _calculateTrancheTokenAmount to find how much tokens the investor will receive.

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

In this function currencyAmountInPriceDecimals is calculated and rounded down when it should be rounded up.

Tools Used

Manual review.

Recommended Mitigation Steps

Round up instead of rounding down to follow the Security Considerations of the standard.

diff --git a/InvestmentManager.sol.orig b/src/InvestmentManager.sol
index fe94965..500ff61 100644
--- a/InvestmentManager.sol.orig
+++ b/src/InvestmentManager.sol
@@ -596,7 +596,7 @@ contract InvestmentManager is Auth {
         (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);
 
         uint256 currencyAmountInPriceDecimals = _toPriceDecimals(currencyAmount, currencyDecimals, liquidityPool).mulDiv(
-            10 ** PRICE_DECIMALS, price, MathLib.Rounding.Down
+            10 ** PRICE_DECIMALS, price, MathLib.Rounding.Up
         );
 
         trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);

This issue can also be found in previewMint function that makes a call to _calculateCurrencyAmount which also round down instead of rounding up.

Assessed type

ERC4626