Submitted by pipidu83, also found by Vagner, KrisApostolov, 0xTiwa, josephdara, 0xgrbr, PENGUN, Kral01, bitsurfer, ether_sky, 0xklh, merlin, maanas, Phantasmagoria, lsaudit, castle_chain, ladboy233, deth, IceBear, 0xPacelli, and MaslarovK
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.
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.
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.
Visual Studio / Manual Review
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:
And be used as
In previewWithdraw and processWithdraw
And
In previewDeposit and processDeposit.
Math
hieronx (Centrifuge) confirmed and commented:
