PuttyV2.sol is allowing the cancelled orders to exercise and withdraw

[code423n4]

Lines of code

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

Vulnerability details

Impact

A cancelled order can be exercised and withdrawn

Proof of Concept

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

    // check floor asset token ids length is 0 unless the position type is put
    !order.isCall
        ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
        : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");

    /* ~~~ EFFECTS ~~~ */

    // send the long position to 0xdead.
    // instead of doing a standard burn by sending to 0x000...000, sending
    // to 0xdead ensures that the same position id cannot be minted again.
    transferFrom(msg.sender, address(0xdead), uint256(orderHash));

    // mark the position as exercised
    exercisedPositions[uint256(orderHash)] = true;

    emit ExercisedOrder(orderHash, floorAssetTokenIds, order);

    /* ~~~ INTERACTIONS ~~~ */

    if (order.isCall) {
        // -- exercising a call option

        // transfer strike from exerciser to putty
        // handle the case where the taker uses native ETH instead of WETH to pay the strike
        if (weth == order.baseAsset && msg.value > 0) {
            // check enough ETH was sent to cover the strike
            require(msg.value == order.strike, "Incorrect ETH amount sent");

            // convert ETH to WETH
            // we convert the strike ETH to WETH so that the logic in withdraw() works
            // - because withdraw() assumes an ERC20 interface on the base asset.
            IWETH(weth).deposit{value: msg.value}();
        } else {
            ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
        }

        // transfer assets from putty to exerciser
        _transferERC20sOut(order.erc20Assets);
        _transferERC721sOut(order.erc721Assets);
        _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
    } else {
        // -- exercising a put option

        // save the floor asset token ids to the short position
        uint256 shortPositionId = uint256(hashOppositeOrder(order));
        positionFloorAssetTokenIds[shortPositionId] = floorAssetTokenIds;

        // transfer strike from putty to exerciser
        ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

        // transfer assets from exerciser to putty
        _transferERC20sIn(order.erc20Assets, msg.sender);
        _transferERC721sIn(order.erc721Assets, msg.sender);
        _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
    }
}

/**
    @notice Withdraws the assets from a short order and also burns the short position 
            that represents it. The assets that are withdrawn are dependent on whether 
            the order is exercised or expired and if the order is a put or call.
    @param order The order to withdraw.
 */
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");

    /* ~~~ EFFECTS ~~~ */

    // send the short position to 0xdead.
    // instead of doing a standard burn by sending to 0x000...000, sending
    // to 0xdead ensures that the same position id cannot be minted again.
    transferFrom(msg.sender, address(0xdead), uint256(orderHash));

    emit WithdrawOrder(orderHash, order);

    /* ~~~ INTERACTIONS ~~~ */

    // 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;
    }

    // transfer assets from putty to owner if put is exercised or call is expired
    if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) {
        _transferERC20sOut(order.erc20Assets);
        _transferERC721sOut(order.erc721Assets);

        // for call options the floor token ids are saved in the long position in fillOrder(),
        // and for put options the floor tokens ids are saved in the short position in exercise()
        uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash);
        _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]);

        return;
    }
}

Tools Used

VS code

Recommended Mitigation Steps

Validation check for cancelled order can be included.

[kirk-baird]

Order's should only be able to be cancelled before they are filled. Maker should not be able to cancel an order after it's filled. Thus exercising and withdrawing should continue to be processed once an order is filled even if it's cancelled.

[outdoteth]

Duplicate: Order can be cancelled even if order was already filled: https://github.com/code-423n4/2022-06-putty-findings/issues/396

[HickupHH3]

Uhhh the warden is asserting the other way round (I think, based on the description). The explanation given by @kirk-baird suffices to explain why this issue should be invalid.