Putty contest - TomJ's results

Table of Contents

Low Risk Issues

• abi.encodePacked should not be used with dynamic types

Non-critical Issues

• Unnecessary Use of Named Returns
• Define Magic Numbers to Constant
• Event is Missing Indexed Fields
• TYPO

Low Risk Issues

[L-01] abi.encodePacked should not be used with dynamic types

Issue

This is because using abi.encodePacked with dynamic types will cause a hash collisions. link: https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode I recommend using abi.encode instead.

PoC

101:    bytes32 public constant ORDER_TYPE_HASH =
102:        keccak256(
103:            abi.encodePacked(
104:                "Order(",
105:                "address maker,",
106:                "bool isCall,",
107:                "bool isLong,",
108:                "address baseAsset,",
109:                "uint256 strike,",
110:                "uint256 premium,",
111:                "uint256 duration,",
112:                "uint256 expiration,",
113:                "uint256 nonce,",
114:                "address[] whitelist,",
115:                "address[] floorTokens,",
116:                "ERC20Asset[] erc20Assets,",
117:                "ERC721Asset[] erc721Assets",
118:                ")",
119:                "ERC20Asset(address token,uint256 tokenAmount)",
120:                "ERC721Asset(address token,uint256 tokenId)"
121:            )
122:        );

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

Mitigation

Use abi.encode instead of abi.encodePacked

 

Non-critical Issues

[N-01] Unnecessary Use of Named Returns

Issue

Several function adds return statement even thought named returns variable are used. Remove unnecessary named returns variable to improve code readability. Also keeping the use of named returns or return statement consistent through out the whole project if possible is recommended.

PoC

1. PuttyV2.sol https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L272 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L363 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L370 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L378

Mitigation

Remove unused named returns variable and keep the use of named returns or return statement consistent through out the whole project if possible.

 

[N-02] Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

./PuttyV2.sol:499:                feeAmount = (order.strike * fee) / 1000;

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

Mitigation

Define magic numbers to constant.

 

[N-03] Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./PuttyV2.sol:171:    event NewBaseURI(string baseURI);
./PuttyV2.sol:177:    event NewFee(uint256 fee);
./PuttyV2.sol:185:    event FilledOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);
./PuttyV2.sol:193:    event ExercisedOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);
./PuttyV2.sol:200:    event WithdrawOrder(bytes32 indexed orderHash, Order order);
./PuttyV2.sol:207:    event CancelledOrder(bytes32 indexed orderHash, Order order);

Mitigation

Use all 3 index fields when possible.

 

[N-04] TYPO

Issue

Some typo was found in the following.

PoC

1. PuttyV2.sol

Change "seperator" to "separator" https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L751

751:        @return The domain seperator used when calculating the eip-712 hash.

Mitigation

Change to correct spelling as mentioned in above PoC

 

[G-01] Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

./PuttyV2.sol:297:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:405:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")

./PuttyV2.sol:395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

./PuttyV2.sol:353:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:429:                require(msg.value == order.strike, "Incorrect ETH amount sent");

./PuttyV2Nft.sol:12:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:27:        require(to != address(0), "INVALID_RECIPIENT");

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

[G-02] Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./PuttyV2.sol:210:        string memory _baseURI,
./PuttyV2.sol:228:    function setBaseURI(string memory _baseURI) public payable onlyOwner {
./PuttyV2.sol:269:        Order memory order,
./PuttyV2.sol:389:    function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
./PuttyV2.sol:466:    function withdraw(Order memory order) public {
./PuttyV2.sol:526:    function cancel(Order memory order) public {
./PuttyV2.sol:547:        Order[] memory orders,
./PuttyV2.sol:574:        Order memory order,
./PuttyV2.sol:576:        Order memory originalOrder
./PuttyV2.sol:593:    function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
./PuttyV2.sol:610:    function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
./PuttyV2.sol:636:    function _transferERC20sOut(ERC20Asset[] memory assets) internal {
./PuttyV2.sol:646:    function _transferERC721sOut(ERC721Asset[] memory assets) internal {
./PuttyV2.sol:657:    function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
./PuttyV2.sol:669:    function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) {
./PuttyV2.sol:683:    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
./PuttyV2.sol:699:    function hashOrder(Order memory order) public view returns (bytes32 orderHash) {
./PuttyV2.sol:727:    function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
./PuttyV2.sol:741:    function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {

Mitigation

Change memory to calldata

 

[G-03] Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

1. PuttyV2.sol

• Cache weth of function fillOrder 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L327 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L351

• Cache fee of function withdraw 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L498-L499

• Cache positionFloorAssetTokenIds of function excercise 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L442 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L448

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

[G-04] Both named returns and return statement are used

Issue

In some function return statement are used even though named returns is set. This is redundant because return statement is not needed when using named returns and named returns is not needed when using return statement. Removing unused named returns variable in below code can save gas and improve code readability.

PoC

1. PuttyV2.sol

Remove returns variable "positionId" https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L272 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L363 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L370 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L378

Mitigation

Remove unused named returns variable as mentioned in above PoC.

 

[G-05] Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this "constant". link for more details: https://github.com/ethereum/solidity/issues/9232

PoC

89:    bytes32 public constant ERC721ASSET_TYPE_HASH =
90:        keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

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

95:    bytes32 public constant ERC20ASSET_TYPE_HASH =
96:        keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

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

101:    bytes32 public constant ORDER_TYPE_HASH =
102:        keccak256(
103:            abi.encodePacked(
104:                "Order(",
105:                "address maker,",
106:                "bool isCall,",
107:                "bool isLong,",
108:                "address baseAsset,",
109:                "uint256 strike,",
110:                "uint256 premium,",
111:                "uint256 duration,",
112:                "uint256 expiration,",
113:                "uint256 nonce,",
114:                "address[] whitelist,",
115:                "address[] floorTokens,",
116:                "ERC20Asset[] erc20Assets,",
117:                "ERC721Asset[] erc721Assets",
118:                ")",
119:                "ERC20Asset(address token,uint256 tokenAmount)",
120:                "ERC721Asset(address token,uint256 tokenId)"
121:            )
122:        );

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

Mitigation

Change the variable from constant to immutable.

 

[G-06] Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

./PuttyV2.sol:497:            uint256 feeAmount = 0;
./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

I suggest removing default value initialization. For example,

• uint feeAmount;
• for (uint256 i; i < orders.length; i++) {

 

[G-07] Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Store array's length as a variable before looping it. For example, I suggest changing it to

    length = orders.length
    for (uint256 i = 0; i < length; i++) {

 

[G-08] ++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

./PuttyV2.sol:556:        for (uint256 i = 0; i < orders.length; i++) {
./PuttyV2.sol:594:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:611:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:627:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:637:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:647:        for (uint256 i = 0; i < assets.length; i++) {
./PuttyV2.sol:658:        for (uint256 i = 0; i < floorTokens.length; i++) {
./PuttyV2.sol:670:        for (uint256 i = 0; i < whitelist.length; i++) {
./PuttyV2.sol:728:        for (uint256 i = 0; i < arr.length; i++) {
./PuttyV2.sol:742:        for (uint256 i = 0; i < arr.length; i++) {

Mitigation

Change it to postfix increments/decrements. For example,

    for (uint256 i = 0; i < orders.length; ++i) {

 

[G-09] != 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in "require" statement.

PoC

./PuttyV2.sol:293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
./PuttyV2.sol:598:            require(token.code.length > 0, "ERC20: Token is not contract");
./PuttyV2.sol:599:            require(tokenAmount > 0, "ERC20: Amount too small");

Mitigation

I suggest changing > 0 to != 0

require(order.baseAsset.code.length != 0, "baseAsset is not contract");

 

[G-10] Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

./PuttyV2.sol:214:        require(_weth != address(0), "Unset weth address");
./PuttyV2.sol:241:        require(_fee < 30, "fee must be less than 3%");
./PuttyV2.sol:278:        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");
./PuttyV2.sol:281:        require(!cancelledOrders[orderHash], "Order has been cancelled");
./PuttyV2.sol:284:        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
./PuttyV2.sol:287:        require(order.duration < 10_000 days, "Duration too long");
./PuttyV2.sol:290:        require(block.timestamp < order.expiration, "Order has expired");
./PuttyV2.sol:293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
./PuttyV2.sol:297:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:298:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");
./PuttyV2.sol:329:                require(msg.value == order.premium, "Incorrect ETH amount sent");
./PuttyV2.sol:353:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:398:        require(order.isLong, "Can only exercise long positions");
./PuttyV2.sol:401:        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
./PuttyV2.sol:405:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
./PuttyV2.sol:406:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
./PuttyV2.sol:429:                require(msg.value == order.strike, "Incorrect ETH amount sent");
./PuttyV2.sol:470:        require(!order.isLong, "Must be short position");
./PuttyV2.sol:475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
./PuttyV2.sol:481:        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
./PuttyV2.sol:527:        require(msg.sender == order.maker, "Not your order");
./PuttyV2.sol:551:        require(orders.length == signatures.length, "Length mismatch in input");
./PuttyV2.sol:552:        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");
./PuttyV2.sol:598:            require(token.code.length > 0, "ERC20: Token is not contract");
./PuttyV2.sol:599:            require(tokenAmount > 0, "ERC20: Amount too small");
./PuttyV2.sol:765:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");
./PuttyV2Nft.sol:12:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:13:        require(_ownerOf[id] == address(0), "ALREADY_MINTED");
./PuttyV2Nft.sol:26:        require(from == _ownerOf[id], "WRONG_FROM");
./PuttyV2Nft.sol:27:        require(to != address(0), "INVALID_RECIPIENT");
./PuttyV2Nft.sol:28-31:        require(msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id], "NOT_AUTHORIZED");
./PuttyV2Nft.sol:41:        require(owner != address(0), "ZERO_ADDRESS");

Mitigation

I suggest implementing custom errors to save gas.