xiaoming90 - Incorrect slippage check during withdraw announcement

[sherlock-admin]

xiaoming90

medium

Incorrect slippage check during withdraw announcement

Summary

Due to an incorrect slippage check during withdraw announcement, the announced order will revert when it is executed, resulting in the user's withdrawal request to fail.

Vulnerability Detail

During withdrawal, the LP has to pay the keeper fee and withdraw fee as per Line 540 below. The total fee (= keeper fee + withdraw fee) is then deducted from the received amount (amountOut), before transferring the collateral (rETH) to the user.

The withdraw fee is calculated as a percentage of the received amount (amountOut) => _withdrawFee = (stableWithdrawFee * _amountOut) / 1e18

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L540

File: DelayedOrder.sol
525:     function _executeStableWithdraw(address account) internal returns (uint256 amountOut) {
526:         FlatcoinStructs.Order memory order = _announcedOrder[account];
527: 
528:         _prepareExecutionOrder(account, order.executableAtTime);
529: 
530:         FlatcoinStructs.AnnouncedStableWithdraw memory stableWithdraw = abi.decode(
531:             order.orderData,
532:             (FlatcoinStructs.AnnouncedStableWithdraw)
533:         );
534: 
535:         uint256 withdrawFee;
536: 
537:         (amountOut, withdrawFee) = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY))
538:             .executeWithdraw(account, order.executableAtTime, stableWithdraw);
539: 
540:         uint256 totalFee = order.keeperFee + withdrawFee;
541: 
542:         // Make sure there is enough margin in the position to pay the keeper fee and withdrawal fee
543:         if (amountOut < totalFee) revert FlatcoinErrors.NotEnoughMarginForFees(int256(amountOut), totalFee);
544: 
545:         // include the fees here to check for slippage
546:         amountOut -= totalFee;

However, within the announceStableWithdraw function, when performing the slippage check using minAmountOut, the code did not take into consideration the withdraw fee. Only the keeper fee is considered as per Line 128. As a result, the expectedAmountOut used during the comparison at Line 130 below will be higher than expected because the trade fee has not been deducted from it. Thus, there is a desync between the expectedAmountOut and the actual received amount.

As a result, if even the actual received amount is less than the minimum amount (minAmountOut), the order will still be announced.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L128

File: DelayedOrder.sol
109:     function announceStableWithdraw(
110:         uint256 withdrawAmount,
111:         uint256 minAmountOut,
112:         uint256 keeperFee
113:     ) external whenNotPaused {
..SNIP..
122:         // Check that the requested minAmountOut is feasible
123:         {
124:             uint256 expectedAmountOut = stableModule.stableWithdrawQuote(withdrawAmount);
125: 
126:             if (keeperFee > expectedAmountOut) revert FlatcoinErrors.WithdrawalTooSmall(expectedAmountOut, keeperFee);
127: 
128:             expectedAmountOut -= keeperFee;
129: 
130:             if (expectedAmountOut < minAmountOut) revert FlatcoinErrors.HighSlippage(expectedAmountOut, minAmountOut);
131: 
132:             vault.checkSkewMax({additionalSkew: expectedAmountOut});
133:         }
134: 
135:         _announcedOrder[msg.sender] = FlatcoinStructs.Order({
136:             orderType: FlatcoinStructs.OrderType.StableWithdraw,
137:             orderData: abi.encode(
138:                 FlatcoinStructs.AnnouncedStableWithdraw({withdrawAmount: withdrawAmount, minAmountOut: minAmountOut})
139:             ),
140:             keeperFee: keeperFee,
141:             executableAtTime: executableAtTime
142:         });

Impact

The announced order will revert when it is executed, resulting in the user's withdrawal request to fail.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L128

Tool used

Manual Review

Recommendation

Consider taking into consideration of the withdraw fee when computing the expected received amount.

- expectedAmountOut -= keeperFee;
+ expectedAmountOut -= (keeperFee + withdrawFee)

if (expectedAmountOut < minAmountOut) revert FlatcoinErrors.HighSlippage(expectedAmountOut, minAmountOut);

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[nevillehuang]

Invalid, this computation is performed here