Centrifuge - ladboy233's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L392 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L405 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L598

Vulnerability details

Impact

Lack of rounding compliance in ERC4626 implementation

Other protocols that integrate with centrifage 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:

Thus, the result of the previewMint and previewWithdraw should be rounded up.

the correct rounding is:

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 ⬇

However, in the current implementation, the previewMnt and previewWithdraw still round down

Tools Used

Manual Review

Recommended Mitigation Steps

previewMint(uint256 shares) - Round Up ⬆

previewWithdraw(uint256 assets) - Round Up ⬆

Assessed type

ERC4626