Putty contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

MEDIUM - functions of the protocol could be impacted

For put options, the fees are not paid as intended.

Proof of Concept

• poc0: fee on non-exercised put option
  • poc0 shows that fee is applied for put option, even though the option was not exercised.
• poc1: no fee on exercised put option
  • poc1 shows that fee is not applied for put option, even though the option was exercised.
• reference case
  • reference shows that the fee is rightfully applied for exercised call option.

As this comment says, fee is applied on exercise.

        @notice Fee rate that is applied on exercise.

But the current implementation applies the fee in withdraw:

	// withdraw function, where the fee is applied	

        // 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;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }


            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);


            return;
        }

Tools Used

foundry

Recommended Mitigation Steps

The fees can be applied in the exercise.

Lines of code

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

Vulnerability details

Impact

HIGH - assets can be lost If a short call order is created with non empty floorTokens array, the taker cannot exercise. Also, the maker cannot withdraw after the expiration. The maker will still get premium when the order is filled. If the non empty floorTokens array was included as an accident, it is a loss for both parties: the taker loses premium without possible exercise, the maker loses the locked ERC20s and ERC721s. This bug is not suitable for exploitation to get a 'free' premium by creating not exercisable options, because the maker will lose the ERC20s and ERC721s without getting any strike. In that sense it is similar but different issue to the Create a short put order with zero tokenAmount makes the option impossible to exercise, therefore reported separately.

Proof of Concept

• proof of concept
• reference case

The proof of concept shows a scenario where babe makes an short call order with non empty floorTokens array. Bob filled the order, and now he has long call option NFT. He wants to exercise his option and calls exercise. There are two cases.

• case 1: he calls exercise with empty floorAssetTokenIds array
• case 2: he calls exercise with non-empty floorAssetTokenIds array with matching length to the orders.floorTokens

In the case1, the input floorAssetTokenIds were checked to be empty for put orders, and his call passes this requirement. But eventually _transferFloorsIn was called and he gets Index out of bounds error, because floorTokens is not empty which does not match with empty floorAssetTokenIds.

// case 1
  // PuttyV2.sol: _transferFloorsIn called by exercise
  // The floorTokens and floorTokenIds do not match the lenghts
  // floorTokens.length is not zero, while floorTokenIds.length is zero

       ERC721(floorTokens[i]).safeTransferFrom(from, address(this), floorTokenIds[i]);

In the case2, the input floorAssetTokenIds were checked to be empty for put orders, but it is not empty. So it reverts.

// case2
// PuttyV2.sol: exercise
// non empty floorAssetTokenIds array is passed for put option, it will revert

        !order.isCall
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

After the option is expired, the maker - babe is trying to withdraw but fails due to the same issue with the case1.

// maker trying to withdraw
// PuttyV2.sol: withdraw

  _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]);

Note on the poc:

• The test for case1 is commented out because foundry could not catch the revert. But by running the test with un-commenting these lines will show that the call reverts with Index out of bounds.
• For the same reason the withdraw also is commented out
• The reference case just shows that it works as intended when the order does not contain non-empty floorTokens.

Tools Used

foundry

Recommended Mitigation Steps

It happens because the fillOrder does not ensure the order.floorTokens to be empty when the order is short call.

Lines of code

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

Vulnerability details

Impact

HIGH - assets can be compromised By creating an order which can be filled but not possible to exercise, the maker of this order is basically getting free premium. Even if the taker wants to exercise the long put option, it will revert. After the option is expired, the maker can withdraw strike. Malicious users can make attractive deals to lure people to take their orders then wait until it expires and takes back all strikes. This will also hurt the reputation of the platform.

It is similar case with Create an short call order with non empty floor makes the option impossible to exercise and withdraw but the root cause is different, also the maker cannot withdraw after expiration.

Proof of Concept

• proof of concept
• reference case without issue

The proof of concept shows a scenario, where babe makes a short put order with 0 tokenAmount. Bob filled the order and he has an NFT of long put. When he calls the exercise, the putty contract tries to transfer ERC20s from bob to itself. However the _transferERC20sIn function reverts, because it requires the tokenAmount to be greater than zero.

// When bob calls exercise
// PuttyV2.sol: _transferERC20sIn function called by exercise
// 	require(tokenAmount > 0, "ERC20: Amount too small");

    function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
        for (uint256 i = 0; i < assets.length; i++) {
            address token = assets[i].token;
            uint256 tokenAmount = assets[i].tokenAmount;


            require(token.code.length > 0, "ERC20: Token is not contract");
            require(tokenAmount > 0, "ERC20: Amount too small");


            ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
        }
    }

Since bob can never exercise his option, babe does not need to worry about the price movement. She can also make attractive deals without worrying it will realize. Basically she is taking free premium. After the option is expired, the maker of the order, babe, can now withdraw the strike (minus fees, based on the current implementation, but the fee should not be paid when not exercised - see the issue The fee is not paid as intended for put orders).

Note:

• In the reference case, the tokenAmount is set to 1, instead of 0 and the taker of the order can exercise as intended.

Tools Used

foundry

Recommended Mitigation Steps

The easy mitigation would be getting rid of the requirement: require(tokenAmount > 0, "ERC20: Amount too small"); Alternatively, ensure the orders to have non zero tokenAmount.

Lines of code

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

Vulnerability details

Impact

HIGH - assets can be lost

When an user is calling fillOrder or exercise functions, if the user sent some ETH accidentally, the ETH can be lost.

Proof of Concept

• proof of concept

In the proof of concept, babe creates a short put order. The baseAsset for this order is set to some erc20 other than WETH. Bob calls fillOrder but sends some ETH accidentally. All transaction passes without failing and Bob lost the ETH he sent in.

This can happen in both fillOrder and exercise. Below is a table for the possible cases (, but not tested).

Tools Used

foundry

Recommended Mitigation Steps

The check for msg.value should be added for cases, where user is not supposed to send any ETH in.

PuttyV2 QA Report

Summary / Impression

• The code is clean and well documented. It was enjoyable to work on.

Low

L01: Cancel the same order will emit event again

github link

One can cancel the same order and emit CancelledOrder again.

    function cancel(Order memory order) public {
        require(msg.sender == order.maker, "Not your order");


        bytes32 orderHash = hashOrder(order);


        // mark the order as cancelled
        cancelledOrders[orderHash] = true;


        emit CancelledOrder(orderHash, order);
    }

Non critical

N01: Usage of magic number

github link

The precision of the fee, 1000, can be stored as a constant for easy referring.

 feeAmount = (order.strike * fee) / 1000;

Out Of Scope

OOS01: Mismatch to the spec file

github link

// spec/withdraw.md

    input                                        │ *check position side is equal to short     │
    ┌─────────────────────────────┐              │                                            │
    │ order: Order                ├──────────────► *check position has expired OR it has been │
    │ floorAssetTokeIds: number[] │              │  exercised                                 │
    └─────────────────────────────┘              │                                            │

The spec for withdraw shows that the function takes two inputs: order and floorAssetTokeIds. But the implementation only takes order as an input.

 function withdraw(Order memory order) public {

OOS02: The frontend error for minus values

link

In the create order, the asset type NFT will allow to go to minus id. The asset type ERC20 will allow to go to minus values. The asset type Floor will error out when the token amount goes to minus. Disclaimer: Using firefox.