No slippage protections on deposit/mint and withdraw/redeem

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L210-L217 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L228-L234

Vulnerability details

Impact

As denoted in the comments of LiquidityPool.convertToShares and LiquidityPool.convertToAssets:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L116-L117 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L123-L124

    ///         The calculation is based on the token price from the most recent epoch retrieved from Centrifuge.
    ///         The actual conversion will likely differ as the price changes between order submission and execution.

If the price is always increasing, linearly or non-linearly, there should not be an issue considering the rule of thumb to the investors is always depositing/minting early and withdrawing/redeeming late.

Nonetheless, the NAV in the Centrifuge pool could make losses due to defaults. It is NOT always associated with the desirably appreciating trend arising from interests paid by the borrowers.

Proof of Concept

Here's a typical scenario: Asset currency = USDC

1. User A calls LiquidityPool.convertToAssets with 10,000 shares and is returned 12,000 USDC.
2. User A calls requestRedeem() with 10,000 shares.
3. Due to a sudden crash arising from massive defaults, the price returned by Centrifuge after the request gets executed drops to 0.9.
4. User A has no option to cancel the deal except to call redeem() before the membership expires.
5. User A suffer a diminished revenue of 12,000 - 9,000 = 3,000 USDC.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding slippage protections to requestDeposit() and requestRedeem() such that the requests will not be executed if the prices returned by Centrifuge is out of the desired ranges specified by the investors. The requests will simply be brought forward to the next epoch while investors can decide whether or not to wait for the next execution or call decreaseDepositRequest() or decreaseRedeemRequest() to cancel the deals.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as duplicate of #243

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)