search

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

4 stars 0 forks source link

EIP-1271 Signature may not be able to split into r, s, v. These signature can't be used with Infinity NFT marketplace at all while it doesn't break any standard. #250

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L421-L425 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/libs/SignatureChecker.sol#L51-L69

Vulnerability details

Impact

EIP-1271 Signature may not be able to split into r, s, v. These signature can't be used with Infinity NFT marketplace at all while it doesn't break any standard.

Proof of Concept

EIP-1271 can also be other length than 64 or 65 bytes. it can even be 128 bytes or 256 bytes or even more or less. The standard doesn't force EIP-1271 signature to be 64 or 65 bytes.

If EIP-1271 is any other length, it is obviously cannot be split into r, s, v.

verifyOrderSig function will be failed to sent entire signature to the EIP-1271 wallet as it is trying to decode for r, s, v and reencode it back. As a result, signature being sent to EIP-1271 wallet will be truncated into 65 bytes. If your signature is shorter than 65 bytes it will be reverted just in the abi.decode. (64 bytes signature that can split into r, s, v is also reverted)

  /**
   * @notice Check whether a user signed order has valid signature
   * @param order the order to verify
   * @return whether order has valid signature
   */
  function verifyOrderSig(OrderTypes.MakerOrder calldata order) external view returns (bool) {
    // Verify the validity of the signature
    (bytes32 r, bytes32 s, uint8 v) = abi.decode(order.sig, (bytes32, bytes32, uint8));
    return SignatureChecker.verify(_hash(order), order.signer, r, s, v, DOMAIN_SEPARATOR);
  }

As SignatureChecker.verify require r, s, v it is forced verifyOrderSig to decode for r, s, v which can be reverted in some case as mentioned above.

Tools Used

Manual

Recommended Mitigation Steps

SignatureChecker.verify should accept entire signature, not just r, s, v.

Use Opensea Seaport signature verification contract that has been fixed, tested and optimized.

https://github.com/ProjectOpenSea/seaport/blob/main/contracts/lib/SignatureVerification.sol

nneverlander commented 2 years ago

this is just a helper function that's not used in the contract

HardlyDifficult commented 2 years ago

True that verifyOrderSig is not used internally, but the same code appears in isOrderValid. This seems like a potential improvement to be made. Lowering risk and merging with the warden's QA report https://github.com/code-423n4/2022-06-infinity-findings/issues/268