```trancheTokenAmount``` should be rounded UP when proceeding to a withdrawal or previewing a withdrawal.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L515 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L396 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L591

Vulnerability details

Impact

This is good practice when implementing the EIP-4626 vault standard as it is more secure to favour the vault than its users in that case. This can also lead to issues down the line for other protocol integrating Centrifuge, that may assume that rounding was handled according to EIP-4626 best practices.

Proof of Concept

When calling the processWithdraw function, the trancheTokenAmount is computed through the _calculateTrancheTokenAmount function, which rounds DOWN the number of shares required to be burnt to receive the currencyAmount payout/withdrawal

/// @dev Processes user's tranche token redemption after the epoch has been executed on Centrifuge.
/// In case user's redempion order was fullfilled on Centrifuge during epoch execution MaxRedeem and MaxWithdraw
/// are increased and LiquidityPool currency can be transferred to user's wallet on calling processRedeem or processWithdraw.
/// Note: The trancheTokenAmount required to fullfill the redemption order was already locked in escrow upon calling requestRedeem and burned upon collectRedeem.
/// @notice trancheTokenAmount return value is type of uint256 to be compliant with EIP4626 LiquidityPool interface
/// @return trancheTokenAmount the amount of trancheTokens redeemed/burned required to receive the currencyAmount payout/withdrawel.
function processWithdraw(uint256 currencyAmount, address receiver, address user)
public
auth
returns (uint256 trancheTokenAmount)
{
address liquidityPool = msg.sender;
uint128 _currencyAmount = _toUint128(currencyAmount);
require(
(_currencyAmount <= orderbook[user][liquidityPool].maxWithdraw && _currencyAmount != 0),
"InvestmentManager/amount-exceeds-withdraw-limits"
);

uint256 redeemPrice = calculateRedeemPrice(user, liquidityPool);
require(redeemPrice != 0, "LiquidityPool/redeem-token-price-0");

uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(_currencyAmount, liquidityPool, redeemPrice);
_redeem(_trancheTokenAmount, _currencyAmount, liquidityPool, receiver, user);
trancheTokenAmount = uint256(_trancheTokenAmount);
}

function _calculateTrancheTokenAmount(uint128 currencyAmount, address liquidityPool, uint256 price)
internal
view
returns (uint128 trancheTokenAmount)
{
(uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);

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

trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
}

As an additional reason the round UP the amount, the computed amount of shares is also used to _decreaseRedemptionLimits, which could potentially lead to a rounded UP remaining redemption limit post withdrawal (note that for the same reason it would we wise to round UP the _currency amount as well when calling _decreaseRedemptionLimits).

The same function is used in the previewWithdraw function, where is should be rounded UP for the same reasons.

/// @return trancheTokenAmount is type of uin256 to support the EIP4626 Liquidity Pool interface
function previewWithdraw(address user, address liquidityPool, uint256 _currencyAmount)
public
view
returns (uint256 trancheTokenAmount)
{
uint128 currencyAmount = _toUint128(_currencyAmount);
uint256 redeemPrice = calculateRedeemPrice(user, liquidityPool);
if (redeemPrice == 0) return 0;

trancheTokenAmount = uint256(_calculateTrancheTokenAmount(currencyAmount, liquidityPool, redeemPrice));
}

Tools Used

Visual Studio / Manual Review

Recommended Mitigation Steps

As the we do not always want to round the amount of shares UP in _calculateTrancheTokenAmount (e.g. when used in previewDeposit or processDeposit the shares amount is correctly rounded DOWN), the function would actually require an extra argument like below:

function _calculateTrancheTokenAmount(uint128 currencyAmount, address liquidityPool, uint256 price, Math.Rounding rounding)
internal
view
returns (uint128 trancheTokenAmount)
{
(uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);

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

trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
}

And be used as

_calculateTrancheTokenAmount(currencyAmount, liquidityPool, redeemPrice, Math.Rounding.Ceil)

In previewWithdraw and processWithdraw

And

_calculateTrancheTokenAmount(_currencyAmount, liquidityPool, depositPrice, Math.Rounding.Floor)

In previewDeposit and processDeposit

Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[hieronx]

I believe this one and https://github.com/code-423n4/2023-09-centrifuge-findings/issues/210 are duplicates.

[c4-sponsor]

hieronx (sponsor) confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[hieronx]

Mitigated in https://github.com/centrifuge/liquidity-pools/pull/166