Platform: Code4rena
Start Date: 02/08/2023
Pot Size: $42,000 USDC
Total HM: 13
Participants: 45
Period: 5 days
Judge: hickuphh3
Total Solo HM: 5
Id: 271
League: ETH
Rank: 17/45
Findings: 1
Award: $300.75
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: SanketKogekar
Also found by: MohammedRizwan, bin2chen, cartlex_, piyushshukla
300.7546 USDC - $300.75
https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationRouter.sol#L63-L80 https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPair.sol#L211-L226
Loss of funds/tokens for the protocol, since block execution is delegated to the block validator without a hard deadline.
The function swapExactAmountOut() from LiquidationRouter.sol and LiquidationPair.sol use these methods to swap tokens:
source.liquidate(_account, tokenIn, swapAmountIn, tokenOut, _amountOut);
and
_liquidationPair.swapExactAmountOut(_receiver, _amountOut, _amountInMax);
Both methods make sure to pass slippage (minimum amount out), but miss to provide the deadline which is crucial to avoid unexpected trades/losses for users and protocol.
Without a deadline, the transaction might be left hanging in the mempool and be executed way later than the user wanted.
That could lead to users/protocol getting a worse price, because a validator can just hold onto the transaction. And when it does get around to putting the transaction in a block
One part of this change is that PoS block proposers know ahead of time if they're going to propose the next block. The validators and the entire network know who's up to bat for the current block and the next one.
This means the block proposers are known for at least 6 minutes and 24 seconds and at most 12 minutes and 48 seconds.
Further reading: https://blog.bytes032.xyz/p/why-you-should-stop-using-block-timestamp-as-deadline-in-swaps
Manual
Let users provide a fixed deadline as param, and also never set deadline to block.timestamp.
Other
#0 - c4-pre-sort
2023-08-07T22:31:11Z
raymondfam marked the issue as low quality report
#1 - raymondfam
2023-08-07T22:31:44Z
Slippage via _amountInMax has already been implemented.
#2 - c4-pre-sort
2023-08-08T02:36:45Z
raymondfam marked the issue as remove high or low quality report
#3 - c4-pre-sort
2023-08-08T02:37:03Z
raymondfam marked the issue as primary issue
#4 - c4-pre-sort
2023-08-08T05:29:04Z
raymondfam marked the issue as high quality report
#5 - c4-sponsor
2023-08-10T19:48:35Z
asselstine marked the issue as sponsor confirmed
#6 - c4-judge
2023-08-12T09:25:13Z
HickupHH3 marked the issue as selected for report
#7 - stalinMacias
2023-08-14T19:18:11Z
Hey @HickupHH3 Can I ask what is the impact of not setting the deadline even if the code is protected against slippage by _amountInMax?
I get that the transaction can be executed at any point in time, but if the output meets the minimum amount specified by the user, what is the damage caused by not setting a deadline?
IMHO, this should be a Q/A because not setting a deadline doesn't really impact the users, they are protected against slippage with the _amountInMax variable.
#8 - HickupHH3
2023-08-15T02:51:44Z
@stalinMacias
The main argument here is the user will lose out on positive slippage if the exchange rate becomes favourable when the tx is included in a block.
As to why it's M severity, it stems from the same finding made in RubiconV2, where the rationale is "when the value lost is guaranteed to be very large and preventing the mistake has a very low cost, it is reasonable to assign a medium risk rating. The closer the cost/benefit ratio gets to zero, the more likely the issue should be rated QA"