Rounding Issues In Certain Functions

[c4-submissions]

Lines of code

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

Vulnerability details

Background

Per EIP 4626’s Security Considerations:

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, the result of the previewMint and previewWithdraw should be rounded up.

Proof of Concept

The current implementation of previewDeposit function will round down the number of shares returned due to this calculation here at function _calculateTrancheTokenAmount

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

ERC 4626 expects the result returned from previewWithdraw function to be rounded up. However the function _calculateTrancheTokenAmount returns it rounded UP.

Also, the function previewMint has the same problem calling the function _calculateCurrencyAmount

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

Impact

Other protocols that integrate with Centrifuge's Pools 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.

Tools Used

VS Code, Manual Review

Recommended Mitigation Steps

Ensure that the rounding of vault’s functions behave as expected. Following are the expected rounding direction for each vault function:

• previewMint(uint256 shares) - Round Up ⬆
• previewWithdraw(uint256 assets) - Round Up ⬆
• previewRedeem(uint256 shares) - Round Down ⬇
• previewDeposit(uint256 assets) - Round Down ⬇
• convertToAssets(uint256 shares) - Round Down ⬇
• convertToShares(uint256 assets) - Round Down ⬇

Since the functions: _calculateTrancheTokenAmount and _calculateCurrencyAmount are used in other functions, maybe the way is to create another functions (or a flag) to round up the shares for previewMint and previewWithdraw.

Assessed type

ERC4626

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #34

[c4-judge]

gzeon-c4 marked the issue as satisfactory