Putty contest - JohnSmith's results

1). Filled order can be canceled

Problem

Filled order can be canceled, which is confusing. Alice may try to cancel order if she does not like it anymore. Order will become canceled, but it does not change anything. Order maker should not be able to cancel it after it is filled.

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L526-L535

Solution

Add a check

bytes32 orderHash = hashOrder(order);
require(ownerOf(uint256(orderHash)), "Order already filled");

2). Event is missing indexed fields

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

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L185 event FilledOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L193 event ExercisedOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);

3). public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L389 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L550 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L577 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L764

1). <array>.length should not be looked up in every loop of a for-loop

Problem

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas)
memory arrays use `MLOAD` (3 gas)
calldata arrays use `CALLDATALOAD` (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

Proof of Concept

There are 10 instances of this issue:

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 for (uint256 i = 0; i < orders.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 for (uint256 i = 0; i < whitelist.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 for (uint256 i = 0; i < arr.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742 for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Create a variable for array length:

uint length = nameOfArray.length;
for(uint i; i < length; ) { ... }

2). ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)

Problem

Prefix increments are cheaper than postfix increments. Saves 6 gas PER LOOP

Proof of Concept

Instances include: There are 10 instances of this issue:

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 for (uint256 i = 0; i < orders.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 for (uint256 i = 0; i < whitelist.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 for (uint256 i = 0; i < arr.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742 for (uint256 i = 0; i < arr.length; i++) {

Mitigation

change i++ to ++i

3). Using private rather than public for constants, saves gas

Problem

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

Proof of Concept

There are 3 instances of this issue: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L89-L90 bytes32 public constant ERC721ASSET_TYPE_HASH = keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L95-L96 bytes32 public constant ERC20ASSET_TYPE_HASH = keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L101-L122 bytes32 public constant ORDER_TYPE_HASH = keccak256( abi.encodePacked( "Order(", "address maker,", "bool isCall,", "bool isLong,", "address baseAsset,", "uint256 strike,", "uint256 premium,", "uint256 duration,", "uint256 expiration,", "uint256 nonce,", "address[] whitelist,", "address[] floorTokens,", "ERC20Asset[] erc20Assets,", "ERC721Asset[] erc721Assets", ")", "ERC20Asset(address token,uint256 tokenAmount)", "ERC721Asset(address token,uint256 tokenId)" ) );

Mitigation

Change public to private

4). Not using the named return variables when a function returns, wastes deployment gas

Proof of Concept

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L345 return positionId;

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L363 return positionId;

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L370 return positionId;

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L378 return positionId;

Mitigation

Remove positionId

5). Unbounded loops with external calls

Problem

If the loop is updating some state variables of a contract, it should be bounded; otherwise, your contract could get stuck if the loop iteration is hitting the block's gas limit. If a loop is consuming more gas than the block's gas limit, that transaction will not be added to the blockchain; in turn, the transaction would revert. Hence, there is a transaction failure. Always consider having bounded loops in which you are updating contract state variables for each iteration.

Proof of Concept

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556-L558 for (uint256 i = 0; i < orders.length; i++) { positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]); }

6). Comparisons with zero for unsigned integers

Problem

> 0 is less efficient than != 0 for unsigned integers.

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas.

Proof of Concept

Instances include: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L293 require(order.baseAsset.code.length > 0, "baseAsset is not contract");

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L327 if (weth == order.baseAsset && msg.value > 0) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L351 if (weth == order.baseAsset && msg.value > 0) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L427 if (weth == order.baseAsset && msg.value > 0) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L498 if (fee > 0) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L598 require(token.code.length > 0, "ERC20: Token is not contract");

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L599 require(tokenAmount > 0, "ERC20: Amount too small");

Mitigation

Replace > 0 with != 0

7). Custom Errors

Problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here https://blog.soliditylang.org/2021/04/21/custom-errors/

Custom errors are defined using the error statement

Proof of Concept

Instances include: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L214 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L241 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L278-L293 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L297-L298 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L329 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L353 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L395-L401 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L405-L406 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L429 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L470 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L475 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L481 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L527 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L551-L552 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L598-L599 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L765

Mitigation

Replace require statements with custom errors.

8). Default value initialization

Problem

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L497 uint256 feeAmount = 0;

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 for (uint256 i = 0; i < orders.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 for (uint256 i = 0; i < whitelist.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 for (uint256 i = 0; i < arr.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742 for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Remove explicit initialization for default values. uint256 feeAmount;

9). Unchecked arithmetic

Problem

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

Proof of Concept

Instances include:

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L503 ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 for (uint256 i = 0; i < orders.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 for (uint256 i = 0; i < assets.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 for (uint256 i = 0; i < floorTokens.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 for (uint256 i = 0; i < whitelist.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 for (uint256 i = 0; i < arr.length; i++) {

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742 for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Place the arithmetic operations in an unchecked block

	for (uint i; i < length;) {
		...
		unchecked{ ++i; }
	}

10). Caching storage variables in memory to save gas

Problem

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

Proof of Concept

Instances include:

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L498-L499 fee is read twice

Mitigation

Create a variable and use it:

uint _fee = fee;
if (_fee > 0) {
    feeAmount = (order.strike * _fee) / 1000;