search

code-423n4 / 2022-11-size-findings

1 stars 0 forks source link

Cancel auction does not get deleted leading to loss of quoteTokens #297

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L391-L410 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L327

Vulnerability details

Impact

A malicious seller can cancel the auction just after it has ended, receive their baseToken back and then call reveal() to make bidders lose their tokens which is sent to address(0).

Since a.data.lowestQuote == type(uint128).max just before reveal() call is made, the check in SizeSealed.cancelAuction() would pass and the seller would receive their baseToken back and seller is set to address(0). The seller can then call reveal() right after, which sends the quoteTokens to address(0), making bidders not able to withdraw baseTokens nor get refunded quoteTokens.

Transfer to address(0) due to solmate's ERC20 standard not checking for address(0) in the transfer call.

Proof of Concept

  1. Charlie has set up an auction and the auctionId has received bids.
  2. Assume the auction filledBase matches the totalBaseAmount
  3. Charlie being malicious calls cancelAuction() after auction has ended and receives the baseTokens back
  4. Charlie then proceeds to reveal the auction, there will be no leftover baseToken to transfer since totalBaseAmount and filledBase match
  5. All quoteTokens would be transferred to address(0) which is now the seller address after auction cancellation.
  6. Bidders are unable to retrieve their quoteTokens nor are they able to withdraw baseTokens

Tools Used

Manual review

Recommended Mitigation Steps

There needs to be some timestamp check in the cancelAuction function as well as deletion of the auctionId during the call. Additionally, there should be a check in finalize() to ensure that the auctionId is active

trust1995 commented 1 year ago

Reveal will not work after cancelAuction is called because a.data.seller is nullified. This is why we POC high severity findings, please make an effort to do that or carefully go over the flow, @cryptphitraining

cryptphitraining commented 1 year ago

Thanks @trust1995 . I initially wanted to use finalize() instead of reveal() in the report but changed it last minute thinking the direct call of finalize() would fail due to the public and private key input needed. But my thoughts were actually on finalising the auction.

I shouldn't have made the last minute change from finialize() to reveal() in the report 😖

Just seeing again that actually reveal checks the seller.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid