previewWithdraw doesn't round up the asset amount leading to confusion when external protocols integrate with the vaults of the protocol

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L338-L347 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L569-L582

Vulnerability details

Impact

Protocols, that integrate with Centrifuge liquidity pools may wrongly assume that withdraw rounds the amount up as per the ERC-4626 specification leading to a wide array or issues for both parties.

Proof of Concept

The following is stated in the EIP's security considerations suggests that any withdraw function should round-up instead of down so that the protocol is fully EIP-4626 compliant:

Finally, EIP-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 favour 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.

The case with the protocol's implementation of the standard is different.

Both in convertToAssets() and _calculatePrice() the amounts get rounded down instead of up:

assets = shares.mulDiv(
    LiquidityPoolLike(liquidityPool).latestPrice(),
    10 ** (PRICE_DECIMALS + trancheTokenDecimals - currencyDecimals),
    // @audit here:
    MathLib.Rounding.Down
);

depositPrice = currencyAmountInPriceDecimals.mulDiv(
    10 ** PRICE_DECIMALS, trancheTokenAmountInPriceDecimals,
  // @audit here:
    MathLib.Rounding.Down
);

Tools Used

Manual review

Recommended Mitigation Steps

Consider changing convertToAssets()'s expression to the following:

assets = shares.mulDiv(
    LiquidityPoolLike(liquidityPool).latestPrice(),
    10 ** (PRICE_DECIMALS + trancheTokenDecimals - currencyDecimals),
    MathLib.Rounding.Up
);

And also consider changing _calculateTrancheTokenAmount() to the following so that it rounds up in the case of it getting called by processWithdraw() or previewWithdraw(), and so that it rounds down in the case of getting called by processDeposit() or previewDeposit().

function _calculateTrancheTokenAmount(uint128 currencyAmount, address liquidityPool, uint256 price, bool roundUp)
    internal
    view
    returns (uint128 trancheTokenAmount)
{

    // ...

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

    // ...
}

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