EIP1271 signature validation would fail due to incorrect `EIP1271_MAGIC_VALUE`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xd9f2a64cf5d48d5ed0f5b8076281837daa0cf55740b79c14a932d781ea3dade9 Severity: medium

Description: Palmera contracts has intended to use EIP1271_MAGIC_VALUE as 0x20c13b0b from Constants.sol contract. This magic value is used check whether the signature provided is valid for the provided hash or not. 0x20c13b0b had been used while drafting the EIP-1271 so its considered as legacy magic value but the ERC-1271 has magic value 0x1626ba7e which should be used across contracts to valiate the signature.

ERC-1271 specifically mentions:

// bytes4(keccak256("isValidSignature(bytes32,bytes)")
  bytes4 constant internal MAGICVALUE = 0x1626ba7e;          @audit // use this magic value

  /**
   * @dev Should return whether the signature provided is valid for the provided hash
   * @param _hash      Hash of the data to be signed
   * @param _signature Signature byte array associated with _hash
   *
   * MUST return the bytes4 magic value 0x1626ba7e when function passes.
   * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
   * MUST allow external calls
   */ 
  function isValidSignature(
    bytes32 _hash, 
    bytes memory _signature)
    public
    view 
    returns (bytes4 magicValue);
}

The above Value is derived from bytes4(keccak256("isValidSignature(bytes32,bytes)").

    bytes4 internal constant MAGIC_VALUE = 0x1626ba7e;

Recommendation\ Consider below changes:

-    /// @dev bytes4(keccak256("isValidSignature(bytes,bytes)")
-    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
+    /// @dev bytes4(keccak256("isValidSignature(bytes32,bytes)").
+    bytes4 internal constant MAGIC_VALUE = 0x1626ba7e;

[alfredolopez80]

ok, i catch up that value we used is a legacy value, but remember we work like a module of Safe Contract and Safe in both version 1.3.0 and 1.4.1 use this value, we need to align ourselves with them

Version Safe contract 1.3.0: https://github.com/safe-global/safe-smart-account/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/interfaces/ISignatureValidator.sol#L6

Version Safe contract 1.4.1.: https://github.com/safe-global/safe-smart-account/blob/bf943f80fec5ac647159d26161446ac5d716a294/contracts/interfaces/ISignatureValidator.sol#L6

so this issue is invalid @0xRizwan

[0xRizwan]

@alfredolopez80 Please check this. Safe supports both legacy as well as ERC1271 magic value for signature validation.

I think the contracts should support both legacy as well as ERC1271 magic values as implemented by safe.

[alfredolopez80]

@alfredolopez80 Please check this. Safe supports both legacy as well as ERC1271 magic value for signature validation.

I think the contracts should support both legacy as well as ERC1271 magic values as implemented by safe.

@0xRizwan the library as you mentiond is a library of PassKey module, is a module additional of Safe, that permit to any safe wallet version, supports multiples way to validate a signature "The contracts in this package use ERC-1271 standard and WebAuthn standard to allow signature verification for WebAuthn credentials using the secp256r1 curve. The contracts in this package are designed to be used with precompiles for signature verification in the supported networks or use any verifier contract implementing the EIP-7212 interface as a fallback mechanism"

so, in not a native part of the Safe contract, like mention before, and is true with this module additional the safe account can support many way to verify a signature, but in our contract we use the native validation of safe contract itself, and prevent the user from having to activate another module to support the functionality

additional this module is not audit yet, and require deploy additional contract by the onwer, like describe here: https://github.com/safe-global/safe-modules/tree/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey

[0xRizwan]

@alfredolopez80 Agreed.