SIZE contest - Josiah's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L163 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L351 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L381 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L439

Vulnerability details

Impact

ERC20 tokens that are either deflationary or re-basing down could have their respective balance change. The balance could become insufficient at the time of withdraw(), refund() or cancel() to the bidders whose funds will be locked due to DOS. The way to take the fund out is to send more quoteToken into the contract which is impractical, causing fund loss to the protocol. And there is no guarantee that until the end time the balance would stay above the needed amount, hence, the lock and loss issues persist.

Proof of Concept

The problem originates here where the contract receives lower than expected amount of tokens from the bidders:

Line 163

        SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

Transferring quoteToken to the bidders in the beginning is going to go through, but as time goes on, the contract balance gets depleted and starts to fail all subsequent transfers. Consequently, withdraw(), refund() or cancel() will revert, locking bidders' fund due to DOS. The situation will transpire whether or not the seller chooses to reveal the private key.

Line 351

        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

Line 381

            SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, refundedQuote);

Line 439

        SafeTransferLib.safeTransfer(ERC20(a.params.quoteToken), msg.sender, b.quoteAmount);

Recommended Mitigation Steps

Disallow variable balance tokens of this nature by implementing the same baseToken sanity check on the quoteToken.

Line 163 could be refactored to:

        uint256 balanceBeforeTransfer = ERC20(a.params.quoteToken).balanceOf(address(this));

        SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

        uint256 balanceAfterTransfer = ERC20(a.params.quoteToken).balanceOf(address(this));

        if (balanceAfterTransfer - balanceBeforeTransfer != quoteAmount) { 
            revert UnexpectedBalanceChange(); 
        }

USE OF BLOCK.TIMESTAMP

Some contract code uses the block.timestamp as part of the calculations and time checks. Nevertheless, timestamps can be slightly altered by miners/validators to favor them in contracts that have logics that depend strongly on them.

Consider taking into account this issue and warning the users that such a scenario could happen. If the alteration of timestamps cannot affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future. Here are the instances found.

Lines 29 - 37 Line 60 Lines 425 - 426

EVENT PARAMETERS SHOULD BE INDEXED

Up to three event parameters should be indexed. This will help filter off the logs in listening for specifically wanted data. Using indexed has the benefit of making the arguments log topics instead of data. There are seven instances found.

Lines 97 -122

USE OF DESCRIPTIVE VARIABLE NAMES

Non-descriptive local variables could make code base difficult to read and navigate. Here are some of the instance found.

Line 86 line 181 Lines 337 - 338 Lines 359 - 360 Lines 418 - 419

SETTING TO DEFAULT VALUES

As documented in the link:

https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=delete#delete

It is recommended using delete whenever there is a need to set state variable to its default value, which has a good side effect of saving gas. Here are the instances found.

Line 379 Line 404 Lines 432 - 435

DOS PREVENTION COULD BE EXPLOITED

It is intuitive to limit the bids of an auction to 1000 to prevent DOS as commented on Line 156. However, without adequate measures, the following conditional check could be exploited by a malicious bidder.

Line 157 - 159

        if (bidIndex >= 1000) {
            revert InvalidState();
        }

This is because a bidder can keep invoking bid() and cancelBid() until bid() begins to revert.

As such, it is recommended that all cancelled bids be removed from the array and retain only the valid ones. One easy way is to have the cancelled bid assigned with the last bid prior to having the last element removed using array.pop().

INCOMPLETE FOR LOOP

The for loop below does not have the last element catered for.

Line 302 - 306

It can be fixed by just having line 302 refactored to:

        for (uint256 i; i < seenBidMap.length; i++) {

MISSING NATSPEC

As documented in the link below:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

It is recommended using a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more.

Here are some of the instance found.

Lines 40 - 48 Lines 97 - 122 Lines 28 - 43 Lines 466 - 480

INSUFFICIENT DOCUMENTATION

The code base lacks code documentation, high-level descriptions, and examples, making the contracts difficult to review and increasing the likelihood of user mistakes. The documentation would benefit from more detail.

Review and properly document the above mentioned aspects of the code base. And, consider writing a formal specification of the protocol.

THRESHOLD LIMIT CHECKS

Certain parameters of the contracts can be configured to edge/extreme values, causing a variety of issues and breaking expected interactions within/between contracts. Here are the six instances found pertaining to critical parameter that lacks robust sanity/threshold/limit checks.

Lines 60 - 82

For instance, the check would pass if timings.endTimestamp is set to only one second greater than block.timestamp in line 60. This could have been prevented if a proper threshold value had been to timings.endTimestamp.

Additionally, a seller could set auctionParams.minimumBidQuote to as close as auctionParams.reserveQuotePerBase * auctionParams.totalBaseAmount, making it impractical for many bidders to afford a bid. This could have been prevented if a proper divider had been added to the R.H.S of equation.

CONDITIONAL CHECKS

Checks should be done as early as possible in a code block to prevent unnecessary code executions prior to it that would not only be reverted but also incur a wastage of gas. Here is one instance found that should at least be inserted before line 148.

Line 155 - 159

SPELLING CORRECTIONS

Line 112

    @ runnning should be corrected to running
    /// @notice Bid on a runnning auction

Line 431

    @ futher should be corrected further
        // Prevent any futher access to this EncryptedBid