SIZE contest - RaymondFam's results

Lines of code

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

Vulnerability details

Impact

The protocol did an excellent job by having the baseToken checked for its code existence whilst filtering off fee-on-transfer token with only one conditional check on lines 103 - 105, considering safeTransfer and safeTransferFrom from the Solmate libraries have been known for not checking the existence of code at the token address. Although the seller would unlikely be choosing a quoteToken that is devoid of a contract in it, e.g. not yet deployed or has been destructed, a deflationary quoteToken might have been chosen unknowingly. This could lead to accounting errors resulting in refund(), withdrwa() and/or cancelBid() to revert for the last batch of bidders due to insufficient contract balance.

Proof of Concept

The instances entailed are listed as follows if the seller chooses to finalize when calling reveal():

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

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

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

The following instance will also affect the last batch of unlucky bidder if the seller chooses to prematurely cancel the auction or not finalizing when calling reveal():

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

Tools Used

Manual inspection.

Recommended Mitigation Steps

As such, a similar check should also be applied on the quoteToken by refactoring line 163 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(); }

Lines of code

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

Vulnerability details

Impact

A bidder could repeatedly call bid() and then cancelBid() to increasingly add elements to a.bid. Before too long, the conditional check on lines 157 - 159 is going to start reverting whenever the next bidder is trying to make a new bid.

Proof of Concept

Line 157        if (bidIndex >= 1000) {
Line 158            revert InvalidState();
Line 159        }

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider replacing the cancelled a.bids[bidIndex] with the last element of a.bids, and then pop the last element. That way, a.bids.length would entail only valid and uncancelled bids, making rooms for new bidders to participate. Here is a suggested code block to be included in cancelBid after transferring b.quoteAmount to the bidder:

a.bids[bidIndex] = a.bids[bids.length - 1];
a.bids.pop();

Use delete to Clear Variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements. Here are some the three instances entailed:

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L432-L435 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L379 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L404

block.timestamp Unreliable

The use of block.timestamp as part of the time checks can be slightly altered by miners/validators to favor them in contracts that have logic strongly dependent 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 some of the instances entailed:

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

Last Array Element Omitted

The following instance of for loop is going to have the last element of the array omitted.

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

Consider refactoring the code line as follows:

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

The following refactoring of code is another alternative albeit less gas efficient though because of the adoption of <= instead of <:

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

Variable Names

Consider making the naming of local variables more verbose and descriptive so all other peer developers would better be able to comprehend the intended statement logic, significantly enhancing the code readability. Here are some of the instances entailed:

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L28 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L86 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L131 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L181 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L221 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L337-L338 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L359-L360 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L392 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L418-L419 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L456

Un-indexed Parameters in Events

Consider indexing parameters for events, serving as logs filter when looking for specifically wanted data. Up to three parameters in an event function can receive the attribute indexed which will cause the respective arguments to be treated as log topics instead of data. There are the instances entailed:

https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L97-L122

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

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

Here are some of the instances entailed that are devoid of useful comments:

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L28-L43 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L466-L480 https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L28-L34 https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L40-L48 https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L63-L68 https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L86-L91 https://github.com/code-423n4/2022-11-size/blob/main/src/interfaces/ISizeSealed.sol#L97-L122

Devoid of System Documentation and Complete Technical Specification

A system’s design specification and supporting documentation should be almost as important as the system’s implementation itself. Users rely on high-level documentation to understand the big picture of how a system works. Without spending time and effort to create palatable documentation, a user’s only resource is the code itself, something the vast majority of users cannot understand. Security assessments depend on a complete technical specification to understand the specifics of how a system works. When a behavior is not specified (or is specified incorrectly), security assessments must base their knowledge in assumptions, leading to less effective review. Maintaining and updating code relies on supporting documentation to know why the system is implemented in a specific way. If code maintainers cannot reference documentation, they must rely on memory or assistance to make high-quality changes. Currently, the only documentation for Growth DeFi is a single README file, as well as code comments.

Early Conditional Checks

Conditional checks should appear as early as possible in a function logic. In the event of a revert, it would avoid incurring gas on code executions coming after the checks. The following code block should execute before updating ebid:

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

Or, better yet, right after Auction a has been cached

Inaccurate Graphical Representation

cliffPercent is meant to be divided by 1e18 to furnish a normalized value between 0 and 1. As such, it should be replaced with cliffAmount on line 31 of CommonTokenMath.sol to better portray the base tokens unlocked at vesting start.

https://github.com/code-423n4/2022-11-size/blob/main/src/util/CommonTokenMath.sol#L31

Additionally, the function logic did not implement any step wise release of vested token periodically. Hence, the graphical staircase should be replaced with a linear straight line to better portray the implication of a continuous function represented in lines 64 to 66.

https://github.com/code-423n4/2022-11-size/blob/main/src/util/CommonTokenMath.sol#L64-L66

Likewise, totalBaseAmount on line 18 shadowing the maximum amount of baseToken to be auctioned in struct AuctionParameters should be replaced with baseAmount to be more inline with the input parameter of tokensAvailableAtTime.

https://github.com/code-423n4/2022-11-size/blob/main/src/util/CommonTokenMath.sol#L18

Inadequate Sanity Checks

The following code block could take in more structured checks by implementing some threshold limits:

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

(timings.endTimestamp <= block.timestamp) would easily be bypassed if timings.endTimestamp == block.timestamp + 1. Consider adding a reasonable amount of threshold to block.timestamp.

In a reverse manner, (timings.startTimestamp >= timings.endTimestamp) could easily be skipped if timings.endTimestamp == timings.startTimestamp + 1 Consider also adding a reasonable amount of threshold to timings.startTimestamp.

Like wise, (timings.endTimestamp > timings.vestingStartTimestamp) and (timings.vestingStartTimestamp > timings.vestingEndTimestamp) could return false by correspondingly having (timings.endTimestamp == timings.vestingStartTimestamp) and (timings.vestingStartTimestamp == timings.vestingEndTimestamp). A threshold should also be added to timings.endTimestamp and timings.vestingStartTimestamp respectively.

As for line 72, consider refactoring it to:

        if (timings.cliffPercent > 1e18 || timings.cliffPercent == 0) {

Minimization of Edge Cases

When ensuring the minimum bid is not more than the total reserve of the auction as commented in line 75, the conditional check would allow seller to set auctionParams.minimumBidQuote to a value as much as auctionParams.reserveQuotePerBase * auctionParams.totalBaseAmount. This could very much limit the number of bidders having the needed resources to trade.

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

Consider adding an appropriate divider to auctionParams.reserveQuotePerBase.

Non-strict inequalities are cheaper than strict ones

In EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =). As an example, consider replacing >= with the strict counterpart > in the following line of code:

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

        if (timings.startTimestamp > timings.endTimestamp - 1) {

Similarly, as an example, consider replacing <= with the strict counterpart < in the following line of code:

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

        if (timings.endTimestamp < block.timestamp + 1) {

|| Costs Less Gas Than Its Equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the following code line may be rewritten as:

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

            if (!(sharedPoint.x != 1 || sharedPoint.y != 1)) continue;

calldata and memory

When running a function we could pass the function parameters as calldata or memory for variables such as strings, bytes, structs, arrays etc. If we are not modifying the passed parameter, we should pass it as calldata because calldata is more gas efficient than memory.

Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. However, it alleviates the compiler from the abi.decode() step that copies each index of the calldata to the memory index, bypassing the cost of 60 gas on each iteration. Here are some of the instances entailed:

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

Use Storage Instead of Memory for Structs/Arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct. Here are some of the instances entailed:

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L148 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L186 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L231 https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L256

Ternary Over if ... else

Using ternary operator instead of the if else statement saves gas. For instance the following code block may be rewritten as:

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

    quotePerBase == data.previousQuotePerBase
        ? {
            if (data.previousIndex > bidIndex) revert InvalidSorting();
        }
        : {
           revert InvalidSorting();
        }

Private/Internal Function Embedded Modifier Reduces Contract Size

Consider having the logic of a modifier embedded through an internal or private (if no contracts inheriting) function to reduce contract size if need be. For instance, the following instance of modifier may be rewritten as follows:

    function _atState(Auction storage a, States _state) private view {
        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();
        }
    }

    modifier atState(Auction storage a, States _state) {
        _atState(a, _state);
        _;
    }

Function Order Affects Gas Consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
  solidity: {
    version: "0.8.17",
    settings: {
      optimizer: {
        enabled: true,
        runs: 1000,
      },
    },
  },
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization PUSH1 0x00 DUP2 EQ ISZERO PUSH1 [cont offset] JUMPI

after optimization DUP1 PUSH1 [revert offset] JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

uint256 Can be Cheaper Than uint8 and Other Unsigned Integer Type of Smaller Bit Size

When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values. Your contract’s gas usage may be higher because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. The EVM needs to properly enforce the limits of this smaller type.

It is only more efficient when you can pack variables of uint8, uint16, uint32, uint64, ... into the same storage slot with other neighboring variables smaller than 32 bytes. Here are some of the instances entailed:

https://github.com/code-423n4/2022-11-size/blob/main/src/util/CommonTokenMath.sol#L48-L50

Unchecked SafeMath Saves Gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ++i ;} to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

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

        for (uint256 i; i < seenBidMap.length - 1; ) {
          if (seenBidMap[i] != type(uint256).max) {
              revert InvalidState();
          }

          unchecked {
              ++i;
          }
        }

abi.encode() Costs More Gas Than abi.encodePacked()

Changing abi.encode() to abi.encodePacked() can save gas considering the former pads extra null bytes at the end of the call data, which is unnecessary. Please visit the following the link delineating how abi.encodePacked() is more gas efficient in general:

https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

Here is one of the instances entailed:

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

Use of Named Returns for Local Variables Saves Gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable. As an example, the following instance of code block can refactored as follows:

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

    function computeCommitment(bytes32 message) public pure returns (bytes32 commitmentHash) {
        commitmentHash = keccak256(abi.encode(message));
    }

+= and -= Costs More Gas

+= generally costs 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop. As an example, the following line of code could be rewritten as:

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

            data.filledBase = data.filledBase + baseAmount;

Calling Internal functions Is Cheaper

From inside a smart contract, calling its internal functions is cheaper than calling its public functions, because when you call a public function, all the parameters are again copied into memory and passed to that function. By contrast, when you call an internal function, references of those parameters are passed and they are not copied into memory again. This saves a bit of gas, especially when the parameters are big.

As such, finalize() should have its visibility changed to internal since calling it internally is the only viable way to have a.data.privKey updated.