Putty contest - 0x1f8b's results

• Low
  • 1. Outdated compiler
  • 2. Undesired approval deletion
  • 3. Lack of ACK during owner change
  • 4. Use encode instead of encodePacked for hashig
  • 5. Use of money laundering
  • 6. Use npm packages instead of copy the dependencies
  • 7. Override all balanceOf logic

---

Low

1. Outdated compiler

The pragma version used are:

pragma solidity 0.8.13;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Examples:

• PuttyV2Nft.sol#L2

2. Undesired approval deletion

getApproved is deleted when someone transfer a token to himself. By not changing ownership of the token, it is not intended or expected to change approvals.

Affected source code:

• PuttyV2Nft.sol#L34

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• PuttyV2.sol#L53

4. Use encode instead of encodePacked for hashig

Use of abi.encodePacked in PuttyV2 is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• PuttyV2.sol#L712-L747

5. Use of money laundering

It is possible to use the contract as a money launderer since it will be the contract that sends the money from possibly illicit activities to a third account.

Example:

• Alice create an short like this:
  • order.isLong = false
  • order.isCall = false
  • order.duration = 0
  • order.expiration = type(uint).max
  • order.maker = Alice
  • order.baseAsset = WETH
  • order.premium = Amount to be laundred
  • order.whitelist.length == 0
  • order.strike = 0
• Bob use the Alice order to call fillOrder with the msg.value = order.premium
• Alice will receive from the market all the expected WETH without paying any fee.

Affected source code:

• PuttyV2.sol#L335-L336
• PuttyV2.sol#L345

6. Use npm packages instead of copy the dependencies

Some contracts use the openzeppelin libraries, as it should be, however these libraries are copied throughout the project instead of use the package manager.

Using the packages from the original developer helps us stay up-to-date when new bugs appear.

Affected source code:

• PuttyV2Nft.sol#L4-L5
• PuttyV2.sol#L39-L44

7. Override all balanceOf logic

The contract PuttyV2Nft is abstract and remove the balanceOf logic in all method except the _burn one.

Although it is not an issue a priori, if the burn logic is changed to not transfer the token to 0xdead address, it could cause unnecessary errors.

Reference:

// send the long position to 0xdead. instead of doing a standard burn by sending to 0x000...000, sending to 0xdead ensures that the same position id cannot be minted again.

It's recommended to override the method _burn like this:

function _burn(uint256 id) internal override {
    address owner = _ownerOf[id];
    require(owner != address(0), "NOT_MINTED");
    delete _ownerOf[id];
    delete getApproved[id];
    emit Transfer(owner, address(0), id);
}

Affected source code:

• PuttyV2Nft.sol#L9
• ERC721.sol#L172-L187

• Gas
  • 1. Unused import
  • 2. Use custom errors instead of revert strings to save Gas
  • 3. Use external visibility
  • 4. Optimize hashOppositeOrder
  • 5. Optimize hashOrder
  • 6. ++i costs less gas compared to i++ or i += 1
  • 7. There's no need to set default values for variables

Gas

1. Unused import

Importing pointless files costs gas during deployment and is a bad coding practice that is important to ignore.

Remove the following imports:

• PuttyV2Nft.sol#L5

2. Use custom errors instead of revert strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Below the affected contract, it is recommended to change all require phrases to custom errors.

• PuttyV2.sol
• PuttyV2Nft.sol

If you decide not to make the mentioned change, it is recommended to shorten all the messages of the require clauses.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described before.

3. Use external visibility

The following methods could be improved if external visibility is used:

• PuttyV2.sol#L272
• PuttyV2.sol#L389
• PuttyV2.sol#L466
• PuttyV2.sol#L526
• PuttyV2.sol#L550
• PuttyV2.sol#L577
• PuttyV2.sol#L669
• PuttyV2.sol#L683
• PuttyV2.sol#L699
• PuttyV2.sol#L727
• PuttyV2.sol#L741

4. Optimize hashOppositeOrder

It's possible to avoid the abi.encode/decode logic around the method hashOppositeOrder doing this change:

function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
    order.isLong = !order.isLong;
    orderHash = hashOrder(order);
    order.isLong = !order.isLong;
}

Affected source code:

• PuttyV2.sol#L683-L690

5. Optimize hashOrder

It's possible to save gas copying all the order fields to abi.encode changing the method hashOrder like this:

function hashOrder(Order memory order) public view returns (bytes32 orderHash) {
    orderHash = keccak256(abi.encode(ORDER_TYPE_HASH, order));
    orderHash = _hashTypedDataV4(orderHash);
}

Affected source code:

• PuttyV2.sol#L699-L720

6. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• PuttyV2.sol#L556
• PuttyV2.sol#L594
• PuttyV2.sol#L611
• PuttyV2.sol#L627
• PuttyV2.sol#L637
• PuttyV2.sol#L647
• PuttyV2.sol#L658
• PuttyV2.sol#L670
• PuttyV2.sol#L728
• PuttyV2.sol#L742

7. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• PuttyV2.sol#L556
• PuttyV2.sol#L594
• PuttyV2.sol#L611
• PuttyV2.sol#L627
• PuttyV2.sol#L637
• PuttyV2.sol#L647
• PuttyV2.sol#L658
• PuttyV2.sol#L670
• PuttyV2.sol#L728
• PuttyV2.sol#L742