Lines of code

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

Vulnerability details

Impact

Function fillOrder suppose to fills an offchain order and settles it onchain. maker signs the orders and give spending allowances to PuttyV2 contract and then taker sends that signed order to this function. but maker can interrupt taker transaction and make it revert. maker can watch miners transaction pools and whenever sees his order is going to be filled in a transaction, he can front-run that transaction and remove PuttyV2 allowance of spending his tokens, then taker transaction would fail in blockchain. there are other ways for maker to perform this attack like reverting on isValidSignatureNow() when order.maker is a contract and PuttyV2 makes an external call to see that signature is valid, maker can revert on that external call based on some logics and cause taker griefing. The real problem is that orders are signed on off-chain orderbook and taker who send them to on-chain contract is in risk of getting rejected transactions and maker can cause this very easily.

Proof of Concept

This is fillOrder() codes:

    /**
        @notice Fills an offchain order and settles it onchain. Mints two
                NFTs that represent the long and short position for the order.
        @param order The order to fill.
        @param signature The signature for the order. Signature must recover to order.maker.
        @param floorAssetTokenIds The floor asset token ids to use. Should only be set 
               when filling a long call order.
        @return positionId The id of the position NFT that the msg.sender receives.
     */
    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);

        /* ~~~ INTERACTIONS ~~~ */

        // transfer premium to whoever is short from whomever is long
        if (order.isLong) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
        } else {
            // handle the case where the user uses native ETH instead of WETH to pay the premium
            if (weth == order.baseAsset && msg.value > 0) {
                // check enough ETH was sent to cover the premium
                require(msg.value == order.premium, "Incorrect ETH amount sent");

                // convert ETH to WETH and send premium to maker
                // converting to WETH instead of forwarding native ETH to the maker has two benefits;
                // 1) active market makers will mostly be using WETH not native ETH
                // 2) attack surface for re-entrancy is reduced
                IWETH(weth).deposit{value: msg.value}();
                IWETH(weth).transfer(order.maker, msg.value);
            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
            }
        }

        // filling short put: transfer strike from maker to contract
        if (!order.isLong && !order.isCall) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
            return positionId;
        }

        // filling long put: transfer strike from taker to contract
        if (order.isLong && !order.isCall) {
            // handle the case where the taker uses native ETH instead of WETH to deposit 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 exercise() works
                // - because exercise() 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);
            }

            return positionId;
        }

        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }
    }

As you can see code tries to transfer some ERC20 or ERC721 tokens from order.maker to contract address and maker can remove spending allowance for PuttyV2 address by front-running and cause the fillOrder() transaction to fail. also in the beginning of function, code uses SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature) and maker can revert the transaction in this call too because when order.maker is a contract, then isValidSignatureNow() makes a external call to order.maker. The steps of this exploit are:

attacker as maker signs an order and put in off-chain order book. (very appealing order)
attacker gives PuttyV2 allowance permission to spend tokens in the orders.
user sees order and tries to fill it by calling fillOrder()
attacker was watching blockchain transactions and perform front-run and remove the allowances of PuttyV2. (or logics of isValidSignatureNow() reverts based on tx.origin)
user transaction fails and user loses gas price.

Tools Used

VIM

Recommended Mitigation Steps

There should be some mechanism in the onchain code that made sure that order signers lose some tokens if their order signature was valid but the spending allowance wasn't correct. makers should stake some gas collateral.

code-423n4 / 2022-06-putty-findings