QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	6
Non-Critical Risk	6

Table of Contents

• 1. Low Risk Issues
  • 1.1. Wrong price on chains other than Ethereum mainnet
  • 1.2. Funds can be locked
  • 1.3. _safeMint() should be used rather than _mint() wherever possible
  • 1.4. A malicious owner can keep the fee rate at zero, but if a large value transfer enters the mempool, the owner can jack the rate up to the maximum
  • 1.5. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 1.6. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. Fee On Transfer Token usage as order.baseAsset in fillOrder() will break exercise()
  • 2.2. Misleading comment
  • 2.3. Typos
  • 2.4. Use string.concat() or bytes.concat()
  • 2.5. It's better to emit after all processing is done
  • 2.6. public functions not called by the contract should be declared external instead

1. Low Risk Issues

1.1. Wrong price on chains other than Ethereum mainnet

PuttyV1 was deployed on both Ethereum and Optimism (native token: OP token). Here, it assumes that msg.value will be in Ether:

src/PuttyV2.sol:
  367:             if (weth == order.baseAsset && msg.value > 0) { 
  368                  // check enough ETH was sent to cover the premium
  369                  require(
  370:                     msg.value == order.premium,
  371                      "Incorrect ETH amount sent"

  377                  // 2) attack surface for re-entrancy is reduced
  378:                 IWETH(weth).deposit{value: msg.value}();
  379:                 IWETH(weth).transfer(order.maker, msg.value);

  401              // handle the case where the taker uses native ETH instead of WETH to deposit the strike
  402:             if (weth == order.baseAsset && msg.value > 0) {
  403                  // check enough ETH was sent to cover the strike
  404:                 require(msg.value == order.strike, "Incorrect ETH amount sent"); 
  405  

  497              // handle the case where the taker uses native ETH instead of WETH to pay the strike
  498:             if (weth == order.baseAsset && msg.value > 0) {
  499                  // check enough ETH was sent to cover the strike
  500:                 require(msg.value == order.strike, "Incorrect ETH amount sent");
  501  

  504                  // - because withdraw() assumes an ERC20 interface on the base asset.
  505:                 IWETH(weth).deposit{value: msg.value}();

This might turn wrong if not careful. Consider checking chainId before making this shortcut.

1.2. Funds can be locked

There isn't a withdraw mechanism and several payable methods are implemented:

PuttyV2.sol:245:    function setBaseURI(string memory _baseURI) public payable onlyOwner { 
PuttyV2.sol:257:    function setFee(uint256 _fee) public payable onlyOwner {
PuttyV2.sol:289:    ) public payable returns (uint256 positionId) {
PuttyV2.sol:450:        payable
PuttyV2.sol:678:    ) public payable returns (uint256 positionId) {

1.3. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

PuttyV2.sol:44:import "solmate/tokens/ERC721.sol";
PuttyV2.sol:337:        _mint(order.maker, uint256(orderHash));
PuttyV2.sol:342:        _mint(msg.sender, positionId);  

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful. The CEIP is already well respected in the solution.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.4. A malicious owner can keep the fee rate at zero, but if a large value transfer enters the mempool, the owner can jack the rate up to the maximum

The PuttyV2.withdraw() function fetches the current fee from the contract's state. This fee can be changed by the Admin/DAO.

File: PuttyV2.sol
257:     function setFee(uint256 _fee) public payable onlyOwner {
258:         require(_fee < 30, "fee must be less than 3%");
259: 
260:         fee = _fee;
261: 
262:         emit NewFee(_fee);
263:     }
264: 

Mitigation: Make sure a governance timelock is in place.

1.5. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

PuttyV2.sol:112:            abi.encodePacked(
PuttyV2.sol:851:                keccak256(abi.encodePacked(order.whitelist)),
PuttyV2.sol:852:                keccak256(abi.encodePacked(order.floorTokens)),
PuttyV2.sol:872:            encoded = abi.encodePacked(
PuttyV2.sol:896:            encoded = abi.encodePacked( 

1.6. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role.
Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

PuttyV2.sol:42:import "openzeppelin/access/Ownable.sol";
PuttyV2.sol:57:    Ownable
PuttyV2.sol:245:    function setBaseURI(string memory _baseURI) public payable onlyOwner { 
PuttyV2.sol:257:    function setFee(uint256 _fee) public payable onlyOwner {

2. Non-Critical Issues

2.1. Fee On Transfer Token usage as order.baseAsset in fillOrder() will break exercise()

As the protocol explicitly mentioned it wouldn't support Fee On Transfer or Rebase tokens, this is just an informative finding. Consider documentation to users that the protocol won't work with such tokens, so they don't fill orders with them.

2.2. Misleading comment

Issue with comment

As an auditor, I personally misunderstood this comment. Consider changing it:

File: PuttyV2.sol
- 273:             * It is also possible to cancel() an order before fillOrder() 
+ 273:             * It is only possible to cancel() an order before fillOrder() 

2.3. Typos

- PuttyV2.sol:910:        @return The domain seperator used when calculating the eip-712 hash. 
+ PuttyV2.sol:910:        @return The domain separator used when calculating the eip-712 hash. 

2.4. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

PuttyV2.sol:2:pragma solidity 0.8.13;
PuttyV2.sol:96:            abi.encodePacked("ERC721Asset(address token,uint256 tokenId)")
PuttyV2.sol:104:            abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)")
PuttyV2.sol:112:            abi.encodePacked(
PuttyV2.sol:851:                keccak256(abi.encodePacked(order.whitelist)),
PuttyV2.sol:852:                keccak256(abi.encodePacked(order.floorTokens)),
PuttyV2.sol:872:            encoded = abi.encodePacked(
PuttyV2.sol:896:            encoded = abi.encodePacked( 

2.5. It's better to emit after all processing is done

• src/PuttyV2.sol:

  354:         emit FilledOrder(orderHash, floorAssetTokenIds, order);
  355  
  356          /* ~~~ INTERACTIONS ~~~ */
  357  
  358          // transfer premium to whoever is short from whomever is long
  359          if (order.isLong) { 
  360              ERC20(order.baseAsset).safeTransferFrom(  

  489:         emit ExercisedOrder(orderHash, floorAssetTokenIds, order);
  490  
  491          /* ~~~ INTERACTIONS ~~~ */
  492  
  493          if (order.isCall) {
  494              // -- exercising a call option
  495  

  576:         emit WithdrawOrder(orderHash, order);
  577  
  578          /* ~~~ INTERACTIONS ~~~ */
  579  
  580          // transfer strike to owner if put is expired or call is exercised
  581          if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
  582              // send the fee to the admin/DAO if fee is greater than 0%

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

PuttyV2.sol:548:    function withdraw(Order memory order) public {
PuttyV2.sol:890:    function encodeERC721Assets(ERC721Asset[] memory arr)  
PuttyV2.sol:912:    function domainSeparatorV4() public view returns (bytes32) {

[outdoteth]

1.3. _safeMint() should be used rather than _mint() wherever possible

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: #327

1.4. A malicious owner can keep the fee rate at zero, but if a large value transfer enters the mempool, the owner can jack the rate up to the maximum

Duplicate: Admin can change fee at any time for existing orders: #422

[outdoteth]

high quality report