SIZE contest - slowmoses's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L155-L159

Vulnerability details

Impact

No auction can accept more than 1000 bids (see code below).

An attacker can see the number of bids already submitted on the chain.

If no bids are submitted yet, the attaker can place a bid at the minimum price and then submit another 999 invalid bids. Now (unless the auction is cancelled) the attacker is guaranteed to be able to buy at the fixed price at the end of the auction.

The attacker has an interest to ALWAYS DO THAT at the beginning of an auction because: The attacker can cancel the real bid if the price becomes unfavourable while the auction is running. This means the attacker gets a free option. The prices of tokens can be quite volatile so the fair value (Black-Scholes) of such an option would be high.

If bids have already been submitted, the attacker might still be interested in buying at a certain price. If so, it is in the attacker's interest to submit invalid bids after the real bid to prevent (normal) competition.

Proof of Concept

uint256 bidIndex = a.bids.length;
// Max of 1000 bids on an auction to prevent DOS
if (bidIndex >= 1000) {
    revert InvalidState();
}

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L155-L159

Tools Used

Manual analysis.

Recommended Mitigation Steps

Remove the limit on bids per auction.

Unreasonably Complex Code

The following code is very hard to read and may or may not contain subtle errors. Consider rewriting, maybe something along the lines of:

if(_state== ABC){
    if(..) revert();
    return;
}
if(_state== DEF){
    if(..) revert();
    return;
}

Instead of:

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) {
    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();
}
_;

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L29-L42

Use Constant

'24 hours' is used in at least three places. This should be a constant.

} else if (block.timestamp <= a.timings.endTimestamp + 24 hours) {
    if (_state != States.RevealPeriod) revert InvalidState();
} else if (block.timestamp > a.timings.endTimestamp + 24 hours) {

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L35-L37

if (a.data.lowestQuote != type(uint128).max || block.timestamp <= a.timings.endTimestamp + 24 hours) {

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L426

Sensitive Terms

Avoid the use of sensitive terms in favor of neutral ones. Use allowlist rather than whitelist.

/// @param proof Merkle proof that checks seller against `merkleRoot` if there is a whitelist

https://github.com/code-423n4/2022-11-size/tree/main/src/SizeSealed.sol#L121

Missing Indexed Fields in Events

Each event should use three indexed fields if there are three or more fields in the event.

---

event AuctionCreated( uint256 auctionId, address seller, AuctionParameters params, Timings timings, bytes encryptedPrivKey ); https://github.com/code-423n4/2022-11-size/tree/main/src/interfaces/ISizeSealed.sol#L97-L99

event Bid( address sender, uint256 auctionId, uint256 bidIndex, uint128 quoteAmount, bytes32 commitment, ECCMath.Point pubKey, bytes32 encryptedMessage, bytes encryptedPrivateKey ); https://github.com/code-423n4/2022-11-size/tree/main/src/interfaces/ISizeSealed.sol#L103-L112

event AuctionFinalized(uint256 auctionId, uint256[] bidIndices, uint256 filledBase, uint256 filledQuote); https://github.com/code-423n4/2022-11-size/tree/main/src/interfaces/ISizeSealed.sol#L118

event Withdrawal(uint256 auctionId, uint256 bidIndex, uint256 withdrawAmount, uint256 remainingAmount); https://github.com/code-423n4/2022-11-size/tree/main/src/interfaces/ISizeSealed.sol#L122

Revert and Require Should Have Descriptive Reason Strings

revert(); https://github.com/code-423n4/2022-11-size/tree/main/src/SizeSealed.sol#L40

Extract Sanity Checks from Loop

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L240-L311

The code linked above and listed at the end of this issue description can be reorganized to require significantly less gas for typical inputs.

There should be two loops. The first should look something like this:

// Bitmap of all the bid indices that have been processed
uint256[] memory seenBidMap = new uint256[]((bidIndices.length/256)+1);


// Fill orders from highest price to lowest price
for (uint256 i; i < bidIndices.length; i++) {
    uint256 bidIndex = bidIndices[i];
    EncryptedBid storage b = a.bids[bidIndex];


    // Verify this bid index hasn't been seen before
    uint256 bitmapIndex = bidIndex / 256;
    uint256 bitMap = seenBidMap[bitmapIndex];
    uint256 indexBit = 1 << (bidIndex % 256);
    if (bitMap & indexBit == 1) revert InvalidState();
    seenBidMap[bitmapIndex] = bitMap | indexBit;

    // Require that bids are passed in descending price
    uint256 quotePerBase = FixedPointMathLib.mulDivDown(b.quoteAmount, type(uint128).max, baseAmount);
    if (quotePerBase >= data.previousQuotePerBase) {
        // If last bid was the same price, make sure we filled the earliest bid first
        if (quotePerBase == data.previousQuotePerBase) {
            if (data.previousIndex > bidIndex) revert InvalidSorting();
        } else {
            revert InvalidSorting();
        }
    }
}

Then we can change the 'continue's in the following code

// Only fill if above reserve price
if (quotePerBase < data.reserveQuotePerBase) continue;


// Auction has been fully filled
if (data.filledBase == data.totalBaseAmount) continue;

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L279-L283

to 'break's and skip going through all the heavy lifting (decrypting..) for a potentially significant part of the bids.

Note that the end result, including all checks, stays the same.

Listing of the original code:

---

// Bitmap of all the bid indices that have been processed
uint256[] memory seenBidMap = new uint256[]((bidIndices.length/256)+1);


// Fill orders from highest price to lowest price
for (uint256 i; i < bidIndices.length; i++) {
    uint256 bidIndex = bidIndices[i];
    EncryptedBid storage b = a.bids[bidIndex];


    // Verify this bid index hasn't been seen before
    uint256 bitmapIndex = bidIndex / 256;
    uint256 bitMap = seenBidMap[bitmapIndex];
    uint256 indexBit = 1 << (bidIndex % 256);
    if (bitMap & indexBit == 1) revert InvalidState();
    seenBidMap[bitmapIndex] = bitMap | indexBit;


    // G^k1^k2 == G^k2^k1
    ECCMath.Point memory sharedPoint = ECCMath.ecMul(b.pubKey, sellerPriv);
    // If the bidder public key isn't on the bn128 curve
    if (sharedPoint.x == 1 && sharedPoint.y == 1) continue;


    bytes32 decryptedMessage = ECCMath.decryptMessage(sharedPoint, b.encryptedMessage);
    // If the bidder didn't faithfully submit commitment or pubkey
    // Or the bid was cancelled
    if (computeCommitment(decryptedMessage) != b.commitment) continue;


    // First 128 bits are the base amount, last are random salt
    uint128 baseAmount = uint128(uint256(decryptedMessage >> 128));


    // Require that bids are passed in descending price
    uint256 quotePerBase = FixedPointMathLib.mulDivDown(b.quoteAmount, type(uint128).max, baseAmount);
    if (quotePerBase >= data.previousQuotePerBase) {
        // If last bid was the same price, make sure we filled the earliest bid first
        if (quotePerBase == data.previousQuotePerBase) {
            if (data.previousIndex > bidIndex) revert InvalidSorting();
        } else {
            revert InvalidSorting();
        }
    }


    // Only fill if above reserve price
    if (quotePerBase < data.reserveQuotePerBase) continue;


    // Auction has been fully filled
    if (data.filledBase == data.totalBaseAmount) continue;


    data.previousQuotePerBase = quotePerBase;
    data.previousIndex = bidIndex;


    // Fill the remaining unfilled base amount
    if (data.filledBase + baseAmount > data.totalBaseAmount) {
        baseAmount = data.totalBaseAmount - data.filledBase;
    }


    b.filledBaseAmount = baseAmount;
    data.filledBase += baseAmount;
}


if (data.previousQuotePerBase != FixedPointMathLib.mulDivDown(clearingQuote, type(uint128).max, clearingBase)) {
    revert InvalidCalldata();
}


// seenBidMap[0:len-1] should be full
for (uint256 i; i < seenBidMap.length - 1; i++) {
    if (seenBidMap[i] != type(uint256).max) {
        revert InvalidState();
    }
}


// seenBidMap[-1] should only have the last N bits set
if (seenBidMap[seenBidMap.length - 1] != (1 << (bidIndices.length % 256)) - 1) {
    revert InvalidState();
}

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L240-L311

Redundant Check

In the following code the check for 'quoteAmount == 0' is included in 'quoteAmount < a.params.minimumBidQuote' and can be removed.

if (quoteAmount == 0 || quoteAmount == type(uint128).max || quoteAmount < a.params.minimumBidQuote) {

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L144