M-1: Some users may not be able to complete their redemption because of a potential underflow
Summary:
The pool.pendingWithdrawalsAmountmay be smaller than a user's redemption amount due to rounding errors which could lead to underflow and block redemption completion.
Description:
User can redeem from the safety module by calling redeem() which will queue a redemption.
Later on they can call completeRedemption() to finalize the redemption and receive their funds from the safety module.
When a slashing happens, the function slash() will first reduce the reservePool_.pendingWithdrawalsAmount (sum of all redemptions) by the percentage of slashing applied to the pool. Then it saves the percentage into the pendingRedemptionAccISFs.
When completing the redemption, the amount sent to the user will be recomputed using the pendingRedemptionAccISFs to apply any slashing that happened during redemption cooldown to the user.
The issue is that the RedemptionLib library that is in charge of computing the slashing over user's and pool's redemption may end up breaking the invariant pool.pendingWithdrawalsAmount < sum(user's redemption_.totalAssetAmount) due to a rounding errors.
This will lead to some last users not being able to complete their redemption as the function will underflow when subtracting their amount from reservePool_.pendingWithdrawalsAmount inside _completeRedemption().
POC:
function test_pendingUserNotGreaterThanPendingTotal(uint256 depositAmount_, uint256 pendingWithdrawalsAmount_, uint256 slashAmount_, uint256 slashAmount2_) public {
pool.depositAmount = bound(depositAmount_, 100 ether, 500_000 ether);
pool.pendingWithdrawalsAmount = bound(pendingWithdrawalsAmount_, 0.1 ether, pool.depositAmount); //can't withdraw more than deposit
uint256 slashAmount1 = bound(slashAmount_, 0.1 ether, pool.depositAmount - 0.1 ether); //can't slash more than deposit - min bound of second slash
uint256 slashAmount2 = bound(slashAmount2_, 0.1 ether, pool.depositAmount - slashAmount1); //can't slash more than deposit - first slash
uint128 userPending = uint128(pool.pendingWithdrawalsAmount); //100% of withdrawals
//slash
pool.pendingWithdrawalsAmount = RedemptionLib.updateRedemptionsAfterTrigger(
pool.pendingWithdrawalsAmount, pool.depositAmount, slashAmount1, reservePoolPendingRedemptionAccISFs
);
pool.depositAmount -= slashAmount1;
console.log("Inverted ratio added by first slash:");
uint256 firstSlashRatio = reservePoolPendingRedemptionAccISFs[0];
emit log_uint(reservePoolPendingRedemptionAccISFs[0]);
//slash again
pool.pendingWithdrawalsAmount = RedemptionLib.updateRedemptionsAfterTrigger(
pool.pendingWithdrawalsAmount, pool.depositAmount, slashAmount2, reservePoolPendingRedemptionAccISFs
);
pool.depositAmount -= slashAmount2;
uint256 newUserPending = RedemptionLib.computeFinalReserveAssetsRedeemed(
reservePoolPendingRedemptionAccISFs, userPending, 1e18, 0
);
console.log("Inverted ratio added by second slash:");
emit log_uint(reservePoolPendingRedemptionAccISFs[0].divWadUp(firstSlashRatio));
console.log("Inverted ratio after second slash:");
emit log_uint(reservePoolPendingRedemptionAccISFs[0]);
console.log("pool.pendingWithdrawalsAmount:");
emit log_uint(pool.pendingWithdrawalsAmount);
console.log("pool.depositAmount:");
emit log_uint(pool.depositAmount);
console.log("newUserPending:");
emit log_uint(newUserPending);
assertGe(pool.pendingWithdrawalsAmount, newUserPending); // WILL REVERT
}
Impact:
Medium. The invariant doesn't hold which can lead to underflow and dos redemption completion.
Recommendation:
Round up the pendingWithdrawalsAmount by modifying oldAssetsPendingRedemption_.mulWadDown(scalingFactor_); to oldAssetsPendingRedemption_.mulWadUp(scalingFactor_); inside computeNewPendingRedemptionsAccumulatedScalingFactor().
Developer Response:
Fixed: https://github.com/Cozy-Finance/cozy-safety-module-private/pull/130.