Putty contest - shung's results

Judge has assessed an item in Issue #190 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L526-L535

Vulnerability details

Impact

Order cancellation requires makers to call cancel(), inputting the order as a function parameter. This is the only cancellation method, and it can cause two issues.

This first issue is that it is an on-chain signal for MEV users to frontrun the cancellation and fill the order.

The second issue is the dependency to a centralized service for cancelling the order. As orders are signed off chain, they would be stored in a centralized database. It is unlikely that an end user would locally record all the orders they make. This means that when cancelling an order, maker needs to request the order parameters from the centralized service. If the centralized service goes offline, it could allow malicious parties who have a copy of the order database to fill orders that would have been cancelled otherwise.

Proof of Concept

1. Bob signs an order which gets recorded in Putty servers.
2. Alice mirrors all the orders using Putty APIs.
3. Putty servers go offline.
4. Bob wants to cancel his order because changing token prices makes his order less favourable to him.
5. Bob cannot cancel his order because Putty servers are down and he does not remember the exact amounts of tokens he used.
6. Alice goes through all the orders in her local mirror and fulfills the non-cancelled orders, including Bob's, with extremely favourable terms for herself.

Tools Used

Pen & paper.

Recommended Mitigation Steps

Aside from the standard order cancellation method, have an extra method to cancel all orders of a caller. This can be achieved using a "minimum valid nonce" state variable, as a mapping from user address to nonce.

mapping(address => uint256) minimumValidNonce;

Allow users to increment their minimumValidNonce. Make sure the incrementation function do not allow incrementing more than 2**64 such that callers cannot lock themselves out of creating orders by increasing minimumValidNonce to 2**256-1 by mistake. Then, prevent filling orders if order.nonce < minimumValidNonce.

Another method to achieve bulk cancelling is using counters. For example, Seaport uses counters, which is an extra order parameter that has to match the corresponding counter state variable. It allows maker to cancel all his orders by incrementing the counter state variable by one.

Either of these extra cancellation methods would enable cancelling orders without signalling to MEV bots, and without a dependency to a centralized database.

QA Report

Trailing whitespaces

Some comments have trailing whitespaces. Simply remove the trailing whitespaces.

diff --git a/contracts/src/PuttyV2.sol b/contracts/src/PuttyV2.sol
index 04cf2fb..91e35fe 100644
--- a/contracts/src/PuttyV2.sol
+++ b/contracts/src/PuttyV2.sol
@@ -3,17 +3,17 @@ pragma solidity 0.8.13;
 
 /**
 
-    ██████╗ ██╗   ██╗████████╗████████╗██╗   ██╗    ██╗   ██╗██████╗ 
+    ██████╗ ██╗   ██╗████████╗████████╗██╗   ██╗    ██╗   ██╗██████╗
     ██╔══██╗██║   ██║╚══██╔══╝╚══██╔══╝╚██╗ ██╔╝    ██║   ██║╚════██╗
     ██████╔╝██║   ██║   ██║      ██║    ╚████╔╝     ██║   ██║ █████╔╝
-    ██╔═══╝ ██║   ██║   ██║      ██║     ╚██╔╝      ╚██╗ ██╔╝██╔═══╝ 
+    ██╔═══╝ ██║   ██║   ██║      ██║     ╚██╔╝      ╚██╗ ██╔╝██╔═══╝
     ██║     ╚██████╔╝   ██║      ██║      ██║        ╚████╔╝ ███████╗
     ╚═╝      ╚═════╝    ╚═╝      ╚═╝      ╚═╝         ╚═══╝  ╚══════╝
-    
-                                
-            _..._               
-          .'     '.      _       
-         /    .-""-\   _/ \ 
+
+
+            _..._
+          .'     '.      _
+         /    .-""-\   _/ \
        .-|   /:.   |  |   |   bussin
        |  \  |:.   /.-'-./
        | .-'-;:__.'    =/
@@ -23,7 +23,7 @@ pragma solidity 0.8.13;
     /   | \    _\  _\
     \__/'._;.  ==' ==\
              \    \   |
-             /    /   / 
+             /    /   /
              /-._/-._/
       jgs    \   `\  \
               `-._/._/
@@ -31,7 +31,7 @@ pragma solidity 0.8.13;
 
     this is a public good.
     by out.eth and tamagoyaki
-    
+
  */
 
 import "./lib/IWETH.sol";
@@ -137,27 +137,27 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
     uint256 public fee;
 
     /**
-        @notice Whether or not an order has been cancelled. Maps 
+        @notice Whether or not an order has been cancelled. Maps
                 from orderHash to isCancelled.
     */
     mapping(bytes32 => bool) public cancelledOrders;
 
     /**
-        @notice The current expiration timestamp of a position. Maps 
+        @notice The current expiration timestamp of a position. Maps
                 from positionId to an expiration unix timestamp.
     */
     mapping(uint256 => uint256) public positionExpirations;
 
     /**
-        @notice Whether or not a position has been exercised. Maps 
+        @notice Whether or not a position has been exercised. Maps
                 from positionId to isExercised.
     */
     mapping(uint256 => bool) public exercisedPositions;
 
     /**
-        @notice The floor asset token ids for a position. Maps from 
-                positionId to floor asset token ids. This should only 
-                be set for a long call position in `fillOrder`, or for 
+        @notice The floor asset token ids for a position. Maps from
+                positionId to floor asset token ids. This should only
+                be set for a long call position in `fillOrder`, or for
                 a short put position in `exercise`.
     */
     mapping(uint256 => uint256[]) public positionFloorAssetTokenIds;
@@ -261,7 +261,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
                 NFTs that represent the long and short position for the order.
         @param order The order to fill.
         @param signature The signature for the order. Signature must recover to order.maker.
-        @param floorAssetTokenIds The floor asset token ids to use. Should only be set 
+        @param floorAssetTokenIds The floor asset token ids to use. Should only be set
                when filling a long call order.
         @return positionId The id of the position NFT that the msg.sender receives.
      */
@@ -380,10 +380,10 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
     }
 
     /**
-        @notice Exercises a long order and also burns the long position NFT which 
+        @notice Exercises a long order and also burns the long position NFT which
                 represents it.
         @param order The order of the position to exercise.
-        @param floorAssetTokenIds The floor asset token ids to use. Should only be set 
+        @param floorAssetTokenIds The floor asset token ids to use. Should only be set
                when exercising a put order.
      */
     function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
@@ -458,8 +458,8 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
     }
 
     /**
-        @notice Withdraws the assets from a short order and also burns the short position 
-                that represents it. The assets that are withdrawn are dependent on whether 
+        @notice Withdraws the assets from a short order and also burns the short position
+                that represents it. The assets that are withdrawn are dependent on whether
                 the order is exercised or expired and if the order is a put or call.
         @param order The order to withdraw.
      */
@@ -562,8 +562,8 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
         @notice Accepts a counter offer for an order. It fills the counter offer, and then
                 cancels the original order that the counter offer was made for.
         @dev There is no need for floorTokenIds here because there is no situation in which
-             it makes sense to have them when accepting counter offers. When accepting a counter 
-             offer for a short call order, the complementary long call order already knows what 
+             it makes sense to have them when accepting counter offers. When accepting a counter
+             offer for a short call order, the complementary long call order already knows what
              tokenIds are used in the short call so floorTokens should always be empty.
         @param order The counter offer to accept.
         @param signature The signature for the counter offer.

Same order can be cancelled twice

cancel() function does not check if the order is already cancelled. This allows CancelledOrder event to be emitted multiple times for the same order, which can cause problems to the frontend and indexers. Consider either reverting, or not emitting an event when order is already cancelled.

diff --git a/contracts/src/PuttyV2.sol b/contracts/src/PuttyV2.sol
index 91e35fe..96fbc70 100644
--- a/contracts/src/PuttyV2.sol
+++ b/contracts/src/PuttyV2.sol
@@ -528,10 +528,12 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own

         bytes32 orderHash = hashOrder(order);

-        // mark the order as cancelled
-        cancelledOrders[orderHash] = true;
+        if (!cancelledOrders[orderHash]) {
+            // mark the order as cancelled
+            cancelledOrders[orderHash] = true;

-        emit CancelledOrder(orderHash, order);
+            emit CancelledOrder(orderHash, order);
+        }
     }

     /* ~~~ PERIPHERY LOGIC FUNCTIONS ~~~ */

Inconsistent event names

Some event names are nouns, some are present tense verbs, and some are past tense verbs. Consider having consistent grammar for event names.

diff --git a/contracts/src/PuttyV2.sol b/contracts/src/PuttyV2.sol
index 91e35fe..feaa96a 100644
--- a/contracts/src/PuttyV2.sol
+++ b/contracts/src/PuttyV2.sol
@@ -168,13 +168,13 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
         @notice Emitted when a new base URI is set.
         @param baseURI The new baseURI.
      */
-    event NewBaseURI(string baseURI);
+    event SetBaseURI(string baseURI);

     /**
         @notice Emitted when a new fee is set.
         @param fee The new fee.
      */
-    event NewFee(uint256 fee);
+    event SetFee(uint256 fee);

     /**
         @notice Emitted when an order is filled.
@@ -197,7 +197,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
         @param orderHash The hash of the order that was withdrawn.
         @param order The order that was withdrawn.
      */
-    event WithdrawOrder(bytes32 indexed orderHash, Order order);
+    event WithdrewOrder(bytes32 indexed orderHash, Order order);

     /**
         @notice emitted When an order is cancelled.
@@ -228,7 +228,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
     function setBaseURI(string memory _baseURI) public payable onlyOwner {
         baseURI = _baseURI;

-        emit NewBaseURI(_baseURI);
+        emit SetBaseURI(_baseURI);
     }

     /**
@@ -242,7 +242,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own

         fee = _fee;

-        emit NewFee(_fee);
+        emit SetFee(_fee);
     }

     /*
@@ -487,7 +487,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
         // to 0xdead ensures that the same position id cannot be minted again.
         transferFrom(msg.sender, address(0xdead), uint256(orderHash));

-        emit WithdrawOrder(orderHash, order);
+        emit WithdrewOrder(orderHash, order);

         /* ~~~ INTERACTIONS ~~~ */

Orders with too many assets or too many whitelisted can remain impossible to fulfill due to block gas limit

Some orders might not get fulfilled if they have too many assets or too many whitelisted addresses. As the number of assets or whitelisted addresses increase, it is possible to reach block gas limit inside the loops, hence there would be DOS on the order. This can cause naive users to waste gas trying to fulfill orders.

Make sure the frontend does not accept creating orders with greater than certain amount of assets and whitelisted addresses.

Providing whitelisted addresses as an array do not appear to be very useful

Due to block gas limit, there can only be very limited number of whitelisted addresses per order. There are not many uses cases for such a limited whitelisting feature.

I suggest using Merkletree proofs to allow whitelisting hundreds of thousands of addresses per order. This enables many use cases such as KYC, blacklisting bad actors by excluding from an almost-all-inclusive whitelist, and whitelisting based on a token balance snapshot.

Return value of WETH transfer is not checked

Although Ethereum WETH reverts on failed transactions, wrapped version of another chains' gas token might not be reverting. It would not do any harm to check the return value of weth transfers.

diff --git a/contracts/src/PuttyV2.sol b/contracts/src/PuttyV2.sol
index 91e35fe..1763952 100644
--- a/contracts/src/PuttyV2.sol
+++ b/contracts/src/PuttyV2.sol
@@ -333,7 +333,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
                 // 1) active market makers will mostly be using WETH not native ETH
                 // 2) attack surface for re-entrancy is reduced
                 IWETH(weth).deposit{value: msg.value}();
-                IWETH(weth).transfer(order.maker, msg.value);
+                require(IWETH(weth).transfer(order.maker, msg.value), "Transfer failed");
             } else {
                 ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
             }