Centrifuge - bitsurfer's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L383-L393

Vulnerability details

Impact

Other protocols that integrate with Centrifuge might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some intergration problem in the future that can lead to wide range of issues for both parties.

Proof of Concept

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.

File: LiquidityPool.sol
160:     function previewMint(uint256 shares) external view returns (uint256 assets) {
161:         assets = investmentManager.previewMint(msg.sender, address(this), shares);
162:     }

File: InvestmentManager.sol
383:     function previewMint(address user, address liquidityPool, uint256 _trancheTokenAmount)
384:         public
385:         view
386:         returns (uint256 currencyAmount)
387:     {
388:         uint128 trancheTokenAmount = _toUint128(_trancheTokenAmount);
389:         uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
390:         if (depositPrice == 0) return 0;
391:
392:         currencyAmount = uint256(_calculateCurrencyAmount(trancheTokenAmount, liquidityPool, depositPrice));
393:     }
...
605:     function _calculateCurrencyAmount(uint128 trancheTokenAmount, address liquidityPool, uint256 price)
606:         internal
607:         view
608:         returns (uint128 currencyAmount)
609:     {
610:         (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);
611:
612:         uint256 currencyAmountInPriceDecimals = _toPriceDecimals(
613:             trancheTokenAmount, trancheTokenDecimals, liquidityPool
614:         ).mulDiv(price, 10 ** PRICE_DECIMALS, MathLib.Rounding.Down);
615:
616:         currencyAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, currencyDecimals, liquidityPool);
617:     }

From LiquidityPool snippet code above, especially for previewMint, the call to InvestmentManager previewMint calculation return a rounding down, while it should be rounding up. This issue also happen in previewWithdraw

reference: https://github.com/code-423n4/2022-06-notional-coop-findings/issues/155

Tools Used

Manual Analysis

Recommended Mitigation Steps

Ensure that the rounding of LiquidityPool vault's functions behave as expected.

previewMint returns the amount of assets that would be deposited to mint specific amount of shares. Thus, the amount of assets must be rounded up, so that the vault won't be shortchanged.

previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the vault won't be shortchanged.

Assessed type

ERC4626