Putty contest - ElKu's results

1) Missing emit statement.

There is a single instance of this in PuttyV2.sol: function acceptCounterOffer

2) Use conventional standard of precision for fee.

Conventionally Basis points is used as a common unit of measure for interest rates and other percentages in finance, which uses 2 decimals of precision rather than 1 decimal as in the project. There seems to be no reason to use a non-conventional standard in the project.

The limit value of fee can be then set to 300, instead of the current 30, in the setFee function. The fee formula should be changed as well as shown below: feeAmount = (order.strike * fee) / 10000;

3) Wrong error message string in function fillOrder()

The error message in line 298 should be same as the one in line 406.

require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

should be

require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

4) Redundant return statement in fillOrder function.

The multiple return positionId statements in fillOrder function is not needed. As positionId is already mentioned as a returned variable in the function declaration part at line 272.

public payable returns (uint256 positionId)

1) Use custom error reporting instead of require statements.

This reduces both deployment and runtime gas usage. See this link for an example.

Also many of these errors are of the same type, which means we can use the same custom error for them, reducing the gas usage even further. The require statements which use the same error message are:

1. File : PuttyV2.sol:

   1. Lines 297-298 and Lines 405-406
   2. Line 329 , Line 353 and Line 429
   3. Line 395 and Line 475
   4. Line 551 and Line 552

2. File : PuttyV2Nft.sol:

   1. Line 12 and Line 27

There are 27 instances in PuttyV2.sol and 6 instances in PuttyV2Nft.sol, where we can use custom error report instead of require statements:

1. File : PuttyV2.sol:

   1. Line 214: require(_weth != address(0), "Unset weth address");
   2. Line 241: require(_fee < 30, "fee must be less than 3%");
   3. Lines 278-298: 8 Require statements
   4. Line 329: require(msg.value == order.premium, "Incorrect ETH amount sent");
   5. Line 353: require(msg.value == order.strike, "Incorrect ETH amount sent");
   6. Lines 395-406: 5 Require statements
   7. Line 429: require(msg.value == order.strike, "Incorrect ETH amount sent");
   8. Line 470: require(!order.isLong, "Must be short position");
   9. Line 475: require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
   10. Line 481: require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
   11. Line 527: require(msg.sender == order.maker, "Not your order");
   12. Lines 551-552: 2 Require statements
   13. Lines 598-599: 2 Require statements
   14. Line 765: require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");

2. File : PuttyV2Nft.sol:

   1. Lines 12-13 : 2 Require statements
   2. Line 26-31: 3 Require statements
   3. Line 41: require(owner != address(0), "ZERO_ADDRESS");

2) The emit statements should be placed at the end of function.

This will save gas in case the function reverts after the emit. Instances where this could be done are:

1. emit FilledOrder(orderHash, floorAssetTokenIds, order); could be either moved to

   1. line 341 or
   2. placed at the end of the function, if we can reformat the 4 individual if statements into an if elseif block. The return statement can be taken out of the if blocks and placed at the end of the function just after the emit statement.

2. emit ExercisedOrder(orderHash, floorAssetTokenIds, order); can be placed at the end of the function(after line 519).

3. emit WithdrawOrder(orderHash, order); can be placed at the end of the function(after line 457).

3) In for loops ++i should be unchecked.

For arithmetic operations which are unlikely to overflow or underflow, you could put them inside an unchecked block. This is usually the case for increment of loop indices in for and while loops.

For example: for (uint256 i = 0; i < orders.length; i++) could be rewritten as:

// you can remove initializing to i as well as its redundant. 
// the length of the array can be cached if the arrays are long, but if its just few elements then you will 
// end up using more gas than before. So I have left it to the sponsor’s judgement.
for (uint256 i; i < orders.length; ) {  
//statements…
    unchecked { 
       i++;
   }
}

There are 10 instances of this in PuttyV2.sol.

1. File : PuttyV2.sol:
   1. Lines 556: for (uint256 i = 0; i < orders.length; i++)
   2. Lines 594: for (uint256 i = 0; i < assets.length; i++)
   3. Lines 611: for (uint256 i = 0; i < assets.length; i++)
   4. Lines 627: for (uint256 i = 0; i < floorTokens.length; i++)
   5. Lines 637: for (uint256 i = 0; i < assets.length; i++)
   6. Lines 647: for (uint256 i = 0; i < assets.length; i++)
   7. Lines 658: for (uint256 i = 0; i < floorTokens.length; i++)
   8. Lines 670: for (uint256 i = 0; i < whitelist.length; i++)
   9. Lines 728: for (uint256 i = 0; i < arr.length; i++)
   10. Lines 742: for (uint256 i = 0; i < arr.length; i++)

4) Combine mutually exclusive if blocks to a single if else block.

In File PuttyV2.sol, these two seperate if statements could be combined into a single if else block. And the return statement could be placed outside the if else block.

The modified code will look like this:

	// we can see that the strike is transferred to owner only when the value of order.isCall and isExercised have different values. 
        // and vice versa for transferring assets from putty to owner. 
	// Both `if blocks` are mutually exclusive. 
        bool flag = order.isCall == isExercised;  
        // transfer strike to owner if put is expired or call is exercised
        if(flag) {
            // send the fee to the admin/DAO if fee is greater than 0%
            uint256 feeAmount = 0;
            if (fee > 0) {
                unchecked { // @audit 
                feeAmount = (order.strike * fee) / 1000;
                }
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);
        }

        // transfer assets from putty to owner if put is exercised or call is expired
        else {
            _transferERC20sOut(order.erc20Assets);
            _transferERC721sOut(order.erc721Assets);

            // for call options the floor token ids are saved in the long position in fillOrder(),
            // and for put options the floor tokens ids are saved in the short position in exercise()
            uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash);
            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]);
        }
	return;

5) The structure elements which are accessed often within a function should be cached.

This saves deployment cost plus execution cost.

There is an instance of this in fillOrder function: order.isCall , order.isLong and order.maker are accessed multiple times inside the fillOrder function. They could be cached like this,

address maker = order.maker;
bool isLong = order.isLong;
bool isCall = order.isCall;

Just the above change caused the average gas consumption for fillOrder function to be reduced by 40(over 51 calls).

Gas Report Before and After Optimizations (except custom reports) are applied:

Below is a comparison of gas costs of few functions in the original code and the modified code(after the above mentioned optimizations are applied). This is without changing the require statements into custom errors. If we add that too, the gas savings will be significant.

File : PuttyV2.sol

Original Gas Report Optimized Code’s gas report Optimized PuttyV2.sol code