Putty contest - dirk_y's results

Lines of code

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

Vulnerability details

Impact

The fillOrder method allows the caller to pay with ETH that is then wrapped to WETH. This only works when the order base asset is WETH. However, if the base asset isn't WETH or if the type of order doesn't transfer value in from the sender, then the transaction doesn't revert or refund the value back to the sender.

I consider this to be of high severity since users of the protocol can accidentally lose funds. A properly functioning protocol should not allow this to occur even by accident.

Proof of Concept

To highlight this behaviour I wrote a new test case in FillOrder.t.sol:

    function testNativeEthCanBeLost() public {
        PuttyV2.Order memory order = defaultOrder();
        order.isLong = true;
        order.isCall = false;
        MockERC20 randomToken = new MockERC20("Mock Token", "MOCK", 18);
        order.baseAsset = address(randomToken);
        bytes memory signature = signOrder(babePrivateKey, order);

        randomToken.mint(address(this), 0xffffffff);
        randomToken.mint(babe, 0xffffffff);
        vm.startPrank(babe);
        randomToken.approve(address(p), type(uint256).max);
        vm.stopPrank();
        randomToken.approve(address(p), type(uint256).max);
        deal(address(this), order.strike);
        uint256 takerBalanceBefore = address(this).balance;
        uint256 contractWethBefore = weth.balanceOf(address(p));
        uint256 contractBalanceBefore = address(p).balance;

        // act
        p.fillOrder{value: order.strike}(order, signature, floorAssetTokenIds);

        // assert
        assertEq(
            weth.balanceOf(address(p)) - contractWethBefore,
            0,
            "No ETH should have been converted to WETH"
        );

        assertEq(
            address(p).balance - contractBalanceBefore,
            order.strike,
            "Taker should have lost ETH"
        );

        assertEq(
            takerBalanceBefore - address(this).balance,
            order.strike,
            "Should have transferred strike ETH from taker to contract"
        );
    }

Tools Used

VSCode + Foundry

Recommended Mitigation Steps

I consider there to be two options to resolve this issue:

1. Revert (or refund) on the non-WETH base asset paths
2. Remove the logic to allow paying with native ETH

I think option 2 is probably the better option given the limited utility the extra logic provides. Especially consider the batchFillOrder method can't be payable given the current logic in fillOrder. By removing the ability to pay with native ETH it will not be possible for users to lose any funds accidentally in this manner. You could also add in a fallback function to the contract to revert on any incoming ETH transfers to prevent any ETH from being locked in the contract by external senders (or alternatively add in an admin only method to withdraw any ETH transferred to the contract).

Lines of code

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

Vulnerability details

Impact

Anyone using American options has to understand their profit/loss potential under certain market conditions. When signing an order, the order maker and filler will be very clear on the profit/loss potential of the order and will be taking into account the protocol fee when setting up the order parameters. However, when an option is exercised and the subsequent parties withdraw from the protocol, the fee percentage is taken as the current fee percentage of the contract rather than the fee when the option was filled. Thus, if the fee percentage were increased then one party of the option would receive less value than expected. The lack of certainty around the fee percentage is an issue for the users of the protocol and provides a path for a potential shock when it comes to time of withdrawal.

Proof of Concept

When withdrawing by calling withdraw the strike is decremented by the fee percentage defined in the fee storage slot. The admin can set the fee variable at any time by calling setFee. Thus it is possible to have a different value in fee between filling the order and withdrawing after order exercise/expiration.

Tools Used

VSCode

Recommended Mitigation Steps

To provide stability for users of the protocol the fee percentage should be fixed at the time the option is filled and saved in storage. Below is a diff with a suggestion for how this could work:

diff --git a/contracts/src/PuttyV2.sol b/contracts/src/PuttyV2.sol
index 04cf2fb..0bb0397 100644
--- a/contracts/src/PuttyV2.sol
+++ b/contracts/src/PuttyV2.sol
@@ -136,6 +136,11 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
     */
     uint256 public fee;
 
+    /**
+        @notice Fee rate when option is filled
+    */
+    mapping(uint256 => uint256) public positionFee;
+
     /**
         @notice Whether or not an order has been cancelled. Maps 
                 from orderHash to isCancelled.
@@ -315,6 +320,9 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
         // save the long position expiration
         positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;
 
+        // save the short position fee percentage
+        positionFee[!order.isLong ? uint256(orderHash) : positionId] = fee;
+
         emit FilledOrder(orderHash, floorAssetTokenIds, order);
 
         /* ~~~ INTERACTIONS ~~~ */
@@ -491,12 +502,14 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
 
         /* ~~~ INTERACTIONS ~~~ */
 
+        uint256 fillFee = positionFee[uint256(orderHash)]
+
         // transfer strike to owner if put is expired or call is exercised
         if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
             // send the fee to the admin/DAO if fee is greater than 0%
             uint256 feeAmount = 0;
-            if (fee > 0) {
-                feeAmount = (order.strike * fee) / 1000;
+            if (fillFee > 0) {
+                feeAmount = (order.strike * fillFee) / 1000;
                 ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
             }

Low severity findings

During my audit of this project I found 2 low severity issues I thought were worth calling out:

1. The docstring for ORDER_TYPE_HASH is incorrect. It should be something like: Order type hash used for EIP-712 encoding.
2. The methods setBaseURI and setFee are payable for no reason. There is no reason why these methods should accept value and it opens up the potential for accidentally locking Ether inside the contract.