QA Report

[code423n4]

• 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

[outdoteth]

1. Outdated compiler

Duplicate: Use of solidity 0.8.13 with known issues in ABI encoding and memory side effects: https://github.com/code-423n4/2022-06-putty-findings/issues/348