Putty contest - StErMi's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L268-L380 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L389-L458

Vulnerability details

Impact

Both fillOrder and exercise functions have the payable keyword because in a particular case they need to be able to receive ETH.

This specific case is when the order.baseAsset is WETH but the user sends ETH. In this case, those ETH are wrapped and used based on the function logic.

The issue is that these functions allow the users to send ETH even if it's we are in the logic flow that is not needed.

When this happens, those ETH will be stuck inside the PuttyV2 contract without an explicit way to recover them.

Proof of Concept

Here is a foundry test case that showcase some scenario for both the fillOrder and exercise function

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "openzeppelin/utils/cryptography/ECDSA.sol";

import "src/PuttyV2.sol";
import "./shared/Fixture.t.sol";

contract TestC4Stuck is Fixture {
    event WithdrawOrder(bytes32 indexed orderHash, PuttyV2.Order order);

    address[] internal floorTokens;
    PuttyV2.ERC20Asset[] internal erc20Assets;
    PuttyV2.ERC721Asset[] internal erc721Assets;
    uint256[] internal floorAssetTokenIds;

    receive() external payable {}

    function setUp() public {
        deal(address(weth), address(this), 0xffffffff);
        deal(address(weth), babe, 0xffffffff);

        weth.approve(address(p), type(uint256).max);

        vm.prank(babe);
        weth.approve(address(p), type(uint256).max);
    }

    function testFillOrderETHStuckIfLong() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.isLong = true;
        bytes memory signature = signOrder(babePrivateKey, order);

        uint256 takerBalanceBefore = weth.balanceOf(address(this));
        uint256 makerBalanceBefore = weth.balanceOf(order.maker);
        uint256 puttyBalanceBefore = address(p).balance;

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

        // Putty has received 1 ether (stuck)
        assertEq(address(p).balance - puttyBalanceBefore, 1 ether);

        // assert
        assertEq(
            weth.balanceOf(address(this)) - takerBalanceBefore,
            order.premium,
            "Should have transferred premium to taker"
        );

        assertEq(
            makerBalanceBefore - weth.balanceOf(order.maker),
            order.premium,
            "Should have transferred premium from maker"
        );
    }

    function testFillOrderETHStuckIfShort() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.baseAsset = address(link);
        order.premium = 1 ether;
        order.isLong = false;
        bytes memory signature = signOrder(babePrivateKey, order);

        // mint 1 link to taker
        link.mint(address(this), order.premium);
        link.approve(address(p), order.premium);

        uint256 takerBalanceBefore = link.balanceOf(address(this));
        uint256 makerBalanceBefore = link.balanceOf(order.maker);
        uint256 puttyBalanceBefore = address(p).balance;

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

        // Putty has received 1 ether (stuck)
        assertEq(address(p).balance - puttyBalanceBefore, 1 ether);

        // assert
        assertEq(
            link.balanceOf(order.maker) - makerBalanceBefore,
            order.premium,
            "Should have transferred premium to maker"
        );

        assertEq(
            takerBalanceBefore - link.balanceOf(address(this)),
            order.premium,
            "Should have transferred premium from taker"
        );
    }

    function testExerciseETHStuckIfPut() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.isLong = false;
        order.isCall = false;
        bytes memory signature = signOrder(babePrivateKey, order);
        p.fillOrder(order, signature, floorAssetTokenIds);
        uint256 balanceBefore = weth.balanceOf(address(this));

        uint256 puttyBalanceBefore = address(p).balance;

        // act
        order.isLong = true;
        p.exercise{value: 1 ether}(order, floorAssetTokenIds);

        // Putty has received 1 ether (stuck)
        assertEq(address(p).balance - puttyBalanceBefore, 1 ether);

        // assert
        assertEq(
            weth.balanceOf(address(this)) - balanceBefore,
            order.strike,
            "Should have transferred strike to exerciser from contract"
        );
    }

    function testExerciseETHStuckIfCall() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.baseAsset = address(link);

        // mint 1 link to taker
        link.mint(address(this), order.strike + order.premium);
        link.approve(address(p), order.strike + order.premium);

        bytes memory signature = signOrder(babePrivateKey, order);
        p.fillOrder(order, signature, floorAssetTokenIds);
        uint256 balanceBefore = link.balanceOf(address(p));

        uint256 puttyBalanceBeforeExercise = address(p).balance;

        // act
        order.isLong = true;
        p.exercise{value: 1 ether}(order, floorAssetTokenIds);

        // Putty has received 1 ether (stuck)
        assertEq(address(p).balance - puttyBalanceBeforeExercise, 1 ether);

        // assert
        assertEq(
            link.balanceOf(address(p)) - balanceBefore,
            order.strike,
            "Should have transferred strike from exerciser to contract"
        );
    }
}

Tools Used

Manual review + forge test

Recommended Mitigation Steps

Putty should revert both fillOrder and exercise in case the user has sent ETH but it was not needed.

For fillOrder msg.value can be greater than > 0

• if order.isLong == false && weth == order.baseAsset
• if order.isLong == true && order.isCall == false && weth == order.baseAsset

For exercise msg.value can be greater than > 0

• if order.isCall == true && weth == order.baseAsset

In all other cases, if msg.value is greater than 0 the contract should revert.

Unused import of OpenZeppelin String.sol in PuttyV2Nft.sol

PuttyV2Nft.sol is importing import "openzeppelin/utils/Strings.sol"; but is never using that library inside the code.

Consider removing it.

PuttyV2Nft.sol miss natspec documentation

Natspec documentation are useful for internal developers that need to work on the project, external developers that need to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

The PuttyV2Nft contract is missing natspec documentation for all the function. A well detailed natspec documentation would be important to describe the current implementation difference for _mint, transferFrom and balanceOf functions.

Consider adding natspec documentation to the contract.

Orders should not be cancellable multiple times or after being filled

The cancelledOrders mapping state variable is only used by fillOrder to prevent to fill an already cancelled order.

To prevent confusion, the protocol should prevent:

1. the order.maker to cancel order multiple times and as a result emitting multiple time the CancelledOrder event
2. the order.maker cancel an order after being filled. Even if it does not impact any logic inside the contract, it still could confuse users interacting directly with the contract/websites that use that information or even with monitoring tools that rely on those events/state variables

Consider preventing order.maker to call the cancel order if the order had been filled or have been already cancelled.

Note: If the protocol implement this solution, they should also consider to "skip" the cancel call inside acceptCounterOffer if the originalOrdr has been cancelled, otherwise the acceptCounterOffer tx would revert.

Use unchecked to increase loop index

Array in solidity are bound to a max length of type(uint256).max this mean that the uint256 i cannot overflow. Because of this reason, it is possible to save some gas with the following modification of the code

-for( uint256 i = 0; i < arr.length; i++ ) {
+for( uint256 i = 0; i < arr.length; ) {

	// loop code
	
+	unchecked {
+		++i;
+	}
}

Here's a diff of forge snapshot before and after the modification

-TestExercise:testItReceivesAssetsFromExerciserForPut() (gas: 702785)
+TestExercise:testItReceivesAssetsFromExerciserForPut() (gas: 701908)

-TestExercise:testItSavesThePositionsFloorTokenIdsIfPut() (gas: 441262)
+TestExercise:testItSavesThePositionsFloorTokenIdsIfPut() (gas: 441199)

-TestExercise:testItSendsAssetsToExerciserForCall() (gas: 689370)
+TestExercise:testItSendsAssetsToExerciserForCall() (gas: 688435)

-TestFillOrder:testItCannotFillOrderIfNotWhitelisted() (gas: 95380)
+TestFillOrder:testItCannotFillOrderIfNotWhitelisted() (gas: 95312)

-TestFillOrder:testItSavesFloorAssetTokenIdsIfLongCall() (gas: 484983)
+TestFillOrder:testItSavesFloorAssetTokenIdsIfLongCall() (gas: 484857)

-TestFillOrder:testItTransfersAssetsFromMakerToPuttyForShortCall() (gas: 428733)
+TestFillOrder:testItTransfersAssetsFromMakerToPuttyForShortCall() (gas: 428191)

-TestFillOrder:testItTransfersAssetsFromTakerToPuttyForLongCall() (gas: 599871)
+TestFillOrder:testItTransfersAssetsFromTakerToPuttyForLongCall() (gas: 599266)

-TestWithdraw:testItWithdrawsAssetsIfCallExpired() (gas: 691460)
+TestWithdraw:testItWithdrawsAssetsIfCallExpired() (gas: 690389)

-TestWithdraw:testItWithdrawsAssetsIfPutExercised() (gas: 758616)
+TestWithdraw:testItWithdrawsAssetsIfPutExercised() (gas: 757273)

Consider adding this gas optimization after further testing gas and security implications.

Avoid to call safeTransfer if feeAmount == 0

In withdraw if order.strike * fee is less than 1000 the value for feeAmount will be 0 because of rounding.

Consider adding a check to prevent a call to safeTransfer if feeAmount equal 0.