code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Vulnerability Details

From https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-encodedata

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

However, it was observed that the Putty encodes in a way that deviates from the EIP-712 Standard. The PuttyV2.encodeERC20Assets and PuttyV2.encodeERC721Assets encode an array by feeding the hash generated from the previous iteration to the hash function of current iteration.

Proof-of-Concept

For example, assume an erc20Assets array with three (3) elements. Following is the pseudo code of the encoding function.

first_hash = hash_function(null, erc20Assets[0])
second_hash = hash_function(first_hash, erc20Assets[1])
third_hash = hash_function(second_hash, erc20Assets[2])

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

    /**
        @notice Hashes an order based on the eip-712 encoding scheme.
        @param order The order to hash.
        @return orderHash The eip-712 compliant hash of the order.
     */
    function hashOrder(Order memory order) public view returns (bytes32 orderHash) {
        orderHash = keccak256(
            abi.encode(
                ORDER_TYPE_HASH,
                order.maker,
                order.isCall,
                order.isLong,
                order.baseAsset,
                order.strike,
                order.premium,
                order.duration,
                order.expiration,
                order.nonce,
                keccak256(abi.encodePacked(order.whitelist)),
                keccak256(abi.encodePacked(order.floorTokens)),
                keccak256(encodeERC20Assets(order.erc20Assets)),
                keccak256(encodeERC721Assets(order.erc721Assets))
            )
        );

        orderHash = _hashTypedDataV4(orderHash);
    }

    /**
        @notice Encodes an array of erc20 assets following the eip-712 encoding scheme.
        @param arr Array of erc20 assets to hash.
        @return encoded The eip-712 encoded array of erc20 assets.
     */
    function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
        for (uint256 i = 0; i < arr.length; i++) {
            encoded = abi.encodePacked(
                encoded,
                keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))
            );
        }
    }

    /**
        @notice Encodes an array of erc721 assets following the eip-712 encoding scheme.
        @param arr Array of erc721 assets to hash.
        @return encoded The eip-712 encoded array of erc721 assets.
     */
    function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {
        for (uint256 i = 0; i < arr.length; i++) {
            encoded = abi.encodePacked(
                encoded,
                keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))
            );
        }
    }

Impact

Hashes generated by Putty does not comply with EIP-712. Thus, the hashes or signatures generated by Putty cannot be parsed by external party since they do not align with the standard, which limit the interoperability of Putty protocol.

Recommended Mitigation Steps

It is recommended to align the encoding scheme with the EIP-712 standard. The array values should be encoded as the keccak256 hash of the concatenated encodeData of their contents/elements.

function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
    bytes32[] memory arrHashes = new bytes32[](
        arr.length
    );
    for (uint256 i = 0; i < arr.length; i++) {
        arrHashes[i] = abi.encodePacked(
            keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))
        );
    }
    return abi.encodePacked(arrHashes);
}

function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {
    bytes32[] memory arrHashes = new bytes32[](
        arr.length
    );
    for (uint256 i = 0; i < arr.length; i++) {
        arrHashes[i] = abi.encodePacked(
            keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))
        );
    }
    return abi.encodePacked(arrHashes);
}

Consider referencing OpenSea's SeaPort implementation of encoding an array according to EIP-712 standard.

_deriveOrderHash - https://github.com/ProjectOpenSea/seaport/blob/a62c2f8f484784735025d7b03ccb37865bc39e5a/reference/lib/ReferenceGettersAndDerivers.sol#L107

_hashOfferItem - https://github.com/ProjectOpenSea/seaport/blob/a62c2f8f484784735025d7b03ccb37865bc39e5a/reference/lib/ReferenceGettersAndDerivers.sol#L36

outdoteth commented 2 years ago

The PuttyV2.encodeERC20Assets and PuttyV2.encodeERC721Assets encode an array by feeding the hash generated from the previous iteration to the hash function of current iteration.

This is not true. Ex - the following code from PuttyV2:

    function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
        for (uint256 i = 0; i < arr.length; i++) {
            encoded = abi.encodePacked(
                encoded,
                // see this line here - the hash from the previous iteration is *not* passed into the hash function
                keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))
            );
        }
    }

HickupHH3 commented 2 years ago

Agree with sponsor, warden is incorrect in this case. It's doing abi.encodePacked(), not keccak256.

Tested in remix to be sure, there isn't a difference. Would have been great for the warden to have done the same before reporting this issue.

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