Seller can steal all the bid fund by repeatedly calling `finalize()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-size/blob/79aa9c01987e57a760521acecfe81b28eab3b313/src/SizeSealed.sol#L327 https://github.com/code-423n4/2022-11-size/blob/79aa9c01987e57a760521acecfe81b28eab3b313/src/SizeSealed.sol#L318-L322

Vulnerability details

Impact

Bidders' fund will be stolen by malicious seller, they can not get refund.

Proof of Concept

In finalize(), FinalizeData memory data is local, which means in each call, data is re-initialized. By calling finalize() multiple times, a malicious seller can get the filledQuote multiple times until all the bid funds are drained.

Even worse, if combined with another vector, by manipulating bidIndices[] at the end, none of the base token will be transferred since the if (quotePerBase < data.reserveQuotePerBase) will continue the loop. The seller can get all the base token back.

The steps are as following:

1. each call to finalize() will receive filledQuote. // src/SizeSealed.sol

   function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote) {
   327:        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), a.data.seller, filledQuote);
   }

2. additionally, to get base token back at last. Call finalize() filled with low bids or fake empty bids, so all the bids will be unsuccessful by if (quotePerBase < data.reserveQuotePerBase) continue;. And all the base token will be considered unsold and returned to the seller.

   function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote) {
       // ...
       if (data.totalBaseAmount != data.filledBase) {
           uint128 unsoldBase = data.totalBaseAmount - data.filledBase;
           a.params.totalBaseAmount = data.filledBase;
           SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), a.data.seller, unsoldBase);
       }

Noted that the key point of this vulnerability is finalize() could be called multiple times, the seller can exploit it. Even without step 2, the seller can still drain the fund. Step 2 is another vector, only make it worse.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• rule to call finalize():

  • only allow one call for finalize()
  • make FinalizeData data a storage variable

• Sanity check for bidIndices[]:

  • enforce the sort of the bidIndices[] by bid price
  • disallow arbitrary input of bidIndices[], only use internal generated sorted bidIndices[]. When create the bid, the array can be sorted in time.
  • or in the if check on line 270, add revert for the case of ascending order

[trust1995]

The warden does not explain how finalize() may be called multiple times, when it is expected that the state will change to finalize following the finalize() call.

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid