Lines of code

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

Vulnerability details

Impact

order.duration is supposed to show the duration that option is valid, but code allows it to be 0, so anyone who take long position with order.duration==0 is going to lose premium without having any chance of exercising that order. positionExpirations[] shows expiration time of orders but the condition in the code for checking that order is expired or not is strict in exercise() and withdraw() so when block.timestamp equals positionExpirations[orderHash] it's not possible to call exercise() or withdraw() and contract logic won't work for that order in the last second. for order.duration==0 case positionExpirationsp would be the same block timestamp that order filled and order would fill but it wouldn't be possible to exercise() that order and the one who took Long position would lose premium.

Proof of Concept

This is fillOrder() code:

    function fillOrder(
        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
        /* ~~~ CHECKS ~~~ */

        bytes32 orderHash = hashOrder(order);

        // check signature is valid using EIP-712
        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");

        // check order is not cancelled
        require(!cancelledOrders[orderHash], "Order has been cancelled");

        // check msg.sender is allowed to fill the order
        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");

        // check duration is valid
        require(order.duration < 10_000 days, "Duration too long");

        // check order has not expired
        require(block.timestamp < order.expiration, "Order has expired");

        // check base asset exists
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

        // check floor asset token ids length is 0 unless the order type is call and side is long
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

        /*  ~~~ EFFECTS ~~~ */

        // create long/short position for maker
        _mint(order.maker, uint256(orderHash));

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

        // save floorAssetTokenIds if filling a long call order
        if (order.isLong && order.isCall) {
            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;
        }

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

        emit FilledOrder(orderHash, floorAssetTokenIds, order);

As you can see it allows filling orders with order.duration == 0 and it sets expiration time as: positionExpirations[] = block.timestamp + order.duration. This is exercise() code:

    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");

As you can see it uses this line: require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired"); to check that position is not expired. and This is withdraw() code:

    function withdraw(Order memory order) public {
        /* ~~~ CHECKS ~~~ */

        // check order is short
        require(!order.isLong, "Must be short position");

        bytes32 orderHash = hashOrder(order);

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

        uint256 longPositionId = uint256(hashOppositeOrder(order));
        bool isExercised = exercisedPositions[longPositionId];

        // check long position has either been exercised or is expired
        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

As you can see it uses this line require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired"); to check to see that if position is expired. Order expiration time is set on fillOrder() and it is checked on exercise() and withdraw() function. In exercise() for checking that orders are not expired, code uses require(block.timestamp < positionExpirations[uint256(orderHash)]) and the condition in withdraw() to check that order is expired is require(block.timestamp > positionExpirations[longPositionId], so when block.timestamp equals to positionExpirationsp[orderHash] those two checks would fail and it wouldn't be possible to perform exercise() and withdraw and for that second contract logic would not work.

So if anyone creates or fills an long position with order duration equal to 0 then s/he is going to lose the premium, because contract allows for those orders to be filled but it doesn't allow them to be exercised, so attacker can create tons of short positions with duration equal to 0 and very preferable for long side(for example put option for BTC at strike price $100K and premium $1K), and if users tries to fill and exercise those orders (get tricked to click on fill and exercise) they would lose the premuim.

And for any contract or code that works with PuttyV2 when block.timestamp==positionExpirations[orderHash] it's not possible to call exercise() or withdraw(). in general in that second user should be able to either all exercise() or withdraw() for that position, but with current logic it's not possible to do it. This bug is preventing one contract to fill and exercise orders in the same block when order.duration is equal to 0 too.

Tools Used

VIM

Recommended Mitigation Steps

prevent orders with order.duration equal to 0 to be filled and also allow one of exercise() or withdraw() when block.timestamp==positionExpirations[orderHash]