Incorrect Implementation of Fallback Function For Signature Verification Failure

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @_AresAudits Submission hash (on-chain): 0x8b5989637fbdeedc5f05fc96212dca593101fdce12bb3e29caf45918aaebe940 Severity: low

Description: Description\ The fallback function in FCLP256Verifier.sol returns an empty string "" when the signature is invalid. This results in a returndatasize of 0, which causes the verifySignatureAllowMalleability function in P256.sol to fail the check eq(returndatasize(), 32). Consequently, the signature verification process fails to correctly identify invalid signatures, leading to potential security vulnerabilities.

Attack Scenario\

1. Setup: A smart contract uses the P256 library to verify signatures for authorizing transactions.
2. Exploit: A User submits a transaction with an invalid signature. The fallback function in FCLP256Verifier.sol returns an empty string "", resulting in a returndatasize of 0. The verifySignatureAllowMalleability function in P256.sol fails the check eq(returndatasize(), 32) and returns false.
3. Impact: The inconsistency in handling return data sizes can lead to confusion and potential issues in other parts of the system that rely on consistent return data sizes.
4. Why This Issue Should Be Fixed: The verifySignatureAllowMalleability function explicitly checks that the return data should be exactly 32 bytes long. This is crucial for ensuring the integrity and correctness of the signature verification process. If the return data size is inconsistent, it can lead to incorrect handling of invalid signatures

Attachments

1. Proof of Concept (PoC) File

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey/contracts/verifiers/FCLP256Verifier.sol#L18C9-L20C10

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey/contracts/verifiers/FCLP256Verifier.sol#L37C8-L39C10

2. Revised Code File (Optional)

   
   // SPDX-License-Identifier: LGPL-3.0-only
   /* solhint-disable no-complex-fallback */
   /* solhint-disable payable-fallback */
   pragma solidity 0.8.24;

import {IP256Verifier} from "../interfaces/IP256Verifier.sol"; import {FCL_ecdsa} from "../vendor/FCL/FCL_ecdsa.sol";

/**

• @title P-256 Elliptic Curve Verifier Based on The FreshCryptoLib P-256 Implementation.

• @custom:security-contact bounty@safe.global */ contract FCLP256Verifier is IP256Verifier { /**

  • @inheritdoc IP256Verifier */ fallback(bytes calldata input) external returns (bytes memory output) { if (input.length != 160) { return abi.encodePacked(0); // Return 32 bytes with a single zero byte }

    bytes32 message; uint256 r; uint256 s; uint256 x; uint256 y;

    // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { message := calldataload(0) r := calldataload(32) s := calldataload(64) x := calldataload(96) y := calldataload(128) }

    if (!FCL_ecdsa.ecdsa_verify(message, r, s, x, y)) { return abi.encodePacked(0); // Return 32 bytes with a single zero byte }

    output = abi.encode(1); } }

To fix this vulnerability, modify the fallback function in FCLP256Verifier.sol to return abi.encodePacked(0) instead of an empty string when the signature is invalid. This ensures that the returndatasize is always 32 bytes, making it easier to handle the return data consistently in the verifySignatureAllowMalleability function in P256.sol. By implementing this change, the verifySignatureAllowMalleability function will correctly handle the return data from the fallback function, ensuring that invalid signatures are properly detected and the verification process is consistent

Files:

• FCLP256Verifier.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmQYGmPQ28PW3dFGkZYsQtv1E5W4afbnjqbFxEwDkmUvgU)

[0xEricTee]

The checks for the return data size and return data value are done in P256.sol Line 124 & Line 126 respectively:

            success := and(
                and(
                    // Return data is exactly 32-bytes long
-->                    eq(returndatasize(), 32),
                    // Return data is exactly the value 0x00..01
-->                    eq(mload(0), 1)
                ),
                // Call does not revert
                staticcall(gas(), verifier, input, 160, 0, 32)
            )

With that being said, I believe this issue is invalid.

[NishantKoyalwar]

Hey @0xEricTee thank u for the response ,yep the line 124-126 checks the returndatasize but the issue here is the fallback function in the FCLP256Verifier.sol does not return bytes32 value for invalid signature instead it returns “”

[0xEricTee]

@NishantKoyalwar Although the return byte sizes are different for valid and invalid signatures, however the code correctly handle all the cases: When the signature is valid, the staticcall must succeed, the return bytes size must be 32 bytes and the return value must be 1, other cases will be evaluated as invalid signatures.

[NishantKoyalwar]

Yep @0xEricTee the code correctly handles for the valid signature m but The inconsistency in handling return data sizes can lead to confusion and potential issues in other parts of the system that rely on consistent return data sizes.therefore submitted this issue as low severity.

[nlordell]

I agree with @0xEricTee that this is invalid.

One additional consideration is the IP256Verifier interface is specified to behave exactly like the EIP-7212 precompile, which only returns 0x00...01 IIF the signature is valid. The current implementation corresponds to how the precompile is actually implemented on multiple chains (e.g. OP and Polygon).

I also think that the interface specified by EIP-7212 is a bit "unclean" (as it could be specified to return 0 padded to 32 bytes in the case an invalid signature, but it isn't). Since the implementation here is intended to follow that of the spec, I will mark this as invalid.

[NishantKoyalwar]

hey @nlordell @0xEricTee ,

Thank you for your feedback, Implementing the fix will ensure consistency in the code and consistent return data sizes, which help in correctly identifying invalid signatures. Below are links to industry-wide implementations of EIP-7212 that follow this approach:

• daimo-eth P256Verifier
• p256.eth.limo mainnet
• p256.eth.limo op-mainnet
• P256Verifier.huff
• P256.eth.limo

Please consider rechecking the issue. If you still think this isn't valid, I will agree with your final decision on this matter.

[nlordell]

As mentioned, the FCLP256Verifier is intended to behave exactly as the EIP-7212/RIP-7212 precompile works at an interface level - including returning the empty string on invalid signatures:

$ curl -s -X POST https://polygon-pokt.nodies.app -H 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id
":42,"method":"eth_call","params":[{"to":"0x0000000000000000000000000000000000000100","data":"0xbb5a52f42f9c9261ed4361f59422a1e30036e7c32b270c8807a419feca6050232ba3a8be6b94d
5ec80a6d9d1190a436effe50d85a1eee859b8cc6af9bd5c2e184cd60b855d442f5b3c7b11eb6c4e0ae7525fe710fab9aa7c77a67f79e6fadd762927b10512bae3eddcfe467828128bad2903269919f7086069c8c4df6c
732838c7787964eaac00e5921fb1498a60f4606766b3d9685001558d1a974e7341513e"},"latest"]}' | jq
{
  "jsonrpc": "2.0",
  "id": 42,
  "result": "0x0000000000000000000000000000000000000000000000000000000000000001"
}
$ curl -s -X POST https://polygon-pokt.nodies.app -H 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id
":42,"method":"eth_call","params":[{"to":"0x0000000000000000000000000000000000000100","data":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000000000000"},"latest"]}' | jq
{
  "jsonrpc": "2.0",
  "id": 42,
  "result": "0x"
}

I understand that other implementations chose to diverge from the P-256 precompile specification, which is up to them - but the one here is explicitly documented to follow it.

[NishantKoyalwar]

Thank you for clarification. I agree with your decision to follow the P-256 precompile specification