code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L278

Vulnerability details

Impact

fillOrder() utilizes SignatureChecker.isValidSignature() to check if the signature is valid.

However, if order.maker is a contract using ERC1271, the contract can temporarily revoke signature in SignatureChecker.isValidSignature(). https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/SignatureChecker.sol

    /**
     * @dev Checks if a signature is valid for a given signer and data hash. If the signer is a smart contract, the
     * signature is validated against that smart contract using ERC1271, otherwise it's validated using `ECDSA.recover`.
     *
     * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus
     * change through time. It could return true at block N and false at block N+1 (or the opposite).
     */
    function isValidSignatureNow(
        address signer,
        bytes32 hash,
        bytes memory signature
    ) internal view returns (bool) {
        (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature);
        if (error == ECDSA.RecoverError.NoError && recovered == signer) {
            return true;
        }

        (bool success, bytes memory result) = signer.staticcall(
            abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
        );
        return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
    }

Proof of Concept

For example, contracts are allowed to view price on market before deciding what to return for isValidSignature, giving them power to temporarily revoke options when market price is not in their favor. This is most likely inappropriate and unintended behaviour for the protocol.

fillOrder() uses SignatureChecker.isValidSignature() to check if the signature is valid https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L278

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

       ...
    }

If order.maker is a contract using EIP1271, it could temporarily/dynamically revoke the signature. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/SignatureChecker.sol

    function isValidSignatureNow(
        address signer,
        bytes32 hash,
        bytes memory signature
    ) internal view returns (bool) {
        …

        (bool success, bytes memory result) = signer.staticcall(
            abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
        );
        return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
    }

Tools Used

None

Recommended Mitigation Steps

Implement signature validation function in PuttyV2.sol.

    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(isValidSignature(order.maker, orderHash, signature), "Invalid signature");

       ...
    }

    function isValidSignature(
        address signer,
        bytes32 hash,
        bytes memory signature
    ) internal view returns (bool) {
        address recovered = ECDSA.recover(hash, signature);
        if (recovered == signer) {
            return true;
        }
        return false;
    }

outdoteth commented 2 years ago

It's intended behaviour of using isValidSignature that a signature can be dynamically revoked. This is no different than a contract cancelling an order or revoking ERC20 approval permissions on an asset to prevent the order being filled.

HickupHH3 commented 2 years ago

agree with sponsor. it's a feature, not bug.

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