Centrifuge - deth's results

Lines of code

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

Vulnerability details

Impact

Per EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

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

Considering the above, previewMint and previewWithdraw should be rounded UP, instead of DOWN.

Currently this won't affect the protocol in any way, but it will affect any third party that wants to integrate with Centrifuge that calls these two methods. The third parties will expect the functions to be implemented by ERC4626 specifications, but since they currently aren't it can lead to problems if the third party relies on them for it's implementation.

You can see a very similar issue here

Proof of Concept

This is the current implementation of previewMint.

    function previewMint(uint256 shares) external view returns (uint256 assets) {
        assets = investmentManager.previewMint(msg.sender, address(this), shares);
    }

It calls investmentManager.previewMint, which in turn calls calculateDepositPrice, which calls _calculatePrice where we can see the following:

        depositPrice = currencyAmountInPriceDecimals.mulDiv(
            10 ** PRICE_DECIMALS, trancheTokenAmountInPriceDecimals, MathLib.Rounding.Down
        );

As you can see we use MathLib.Rounding.Down.

Also _calculateCurrencyAmount which is being called inside previewMint also uses MathLib.Rounding.Down.

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

The exact same issue occurs in previewWithdraw, but it calls calculateRedeemPrice, which in turn calls _calculatePrice which we already saw, uses MathLib.Rounding.Down and inside previewWithdraw we also call _calculateTrancheTokenAmount, which also uses MathLib.Rounding.Down.

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

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend adding a new parameter for all the above functions: roundingDirection. Then for each specific method that uses MathLib.Rounding, pass the direction in which you want the function to round.

Assessed type

ERC4626