Putty contest - Critical's results

Lines of code

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

Vulnerability details

Unlike other American Option systems, which the expiration time is set by the maker, in PuttyV2, the maker will set a duration and an order expiration time (order.expiration).

This means that the final expiration time of the option will be decided by the taker.

For a short order, it's preferred for the taker to fulfill the order later than sooner, as later means the option will be valid for a longer time.

Furthermore, the current implementation allows the maker to set the duration to an arbitrary value, which can be very short or even 0, renders the option useless as it will expired immediately:

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

    // save the long position expiration
    positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;

The option's expiration time (positionExpirations) is base on block.timestamp and order.duration, when order.duration == 0, the option's expiration time will be block.timestamp.

Even if exercise() is called within the same block will revert with "Position has expired".

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

function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check user owns the position
        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");

        // check position is long
        require(order.isLong, "Can only exercise long positions");

        // check position has not expired
        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");

Proof of Concept

Given:

• Punk#100 market price = 100 ETH

1. Alice created an off-chain maker order for a short call option:

• erc721Assets = Punk#100
• premium = 1 ETH
• strike = 50 ETH
• duration = 0

2. Bob filled Alice‘s order:

• 1 ETH transferred from Bob to Alice
• Punk#100 transferred from Alice to contract
• Bob received a long position
• Alice received a short position

3. Bob tried to exercise() immediately, the transaction reverted with error: "Position has expired"

4. Alice called withdraw() and retrieved the Punk#100

Recommended Mitigation Steps

1. Change to maker decide option's expiration time model:

• Removing order.duration and use order.expiration for the expiration time of the option;
• Adding an new order.deadline for the order's expiration time, and the maker order can only be filled before that time;
• Adding a check to ensure order.deadline < order.expiration.

2. Keep the current design:

• Adding a check to ensure order.duration > MIN_DURATION.

The MIN_DURATION should be meaningful for a regular end-user to exercise the option even when the network is congested, eg, 12 hours.