Correct EIP-7212 Behaviour

safe-global · Mar 19, 2024 · cb99a85 · cb99a85
1 parent 24e22f8
commit cb99a85
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 19 deletions.
diff --git a/modules/passkey/contracts/interfaces/IP256Verifier.sol b/modules/passkey/contracts/interfaces/IP256Verifier.sol
@@ -18,9 +18,9 @@ interface IP256Verifier {
      * - `input[ 96:128]`: public key x
      * - `input[128:160]`: public key y
      *
-     * The output is a Solidity ABI encoded boolean value indicating whether or not the signature is
-     * valid. Specifically, it returns 32 bytes with a value of `0x00..00` or `0x00..01` for an
-     * invalid or valid signature respectively.
+     * The output is either:
+     * - `abi.encode(true)` bytes for a valid signature
+     * - `""` empty bytes for an invalid signature or error
      *
      * Note that this function does not follow the Solidity ABI format (in particular, it does not
      * have a 4-byte selector), which is why it requires a fallback function and not regular

diff --git a/modules/passkey/contracts/libraries/P256.sol b/modules/passkey/contracts/libraries/P256.sol
@@ -81,6 +81,7 @@ library P256 {
 
             // Perform staticcall and check result, note that Yul evaluates expressions from right
             // to left. See <https://docs.soliditylang.org/en/v0.8.24/yul.html#function-calls>
+            mstore(0, 0)
             success := and(
                 and(
                     // Return data is exactly 32-bytes long

diff --git a/modules/passkey/contracts/verifiers/FCLP256Verifier.sol b/modules/passkey/contracts/verifiers/FCLP256Verifier.sol
@@ -16,7 +16,7 @@ contract FCLP256Verifier is IP256Verifier {
      */
     fallback(bytes calldata input) external returns (bytes memory output) {
         if (input.length != 160) {
-            return abi.encodePacked(uint256(0));
+            return "";
         }
 
         bytes32 message;
@@ -34,6 +34,10 @@ contract FCLP256Verifier is IP256Verifier {
             y := calldataload(128)
         }
 
-        output = abi.encode(FCL_ecdsa.ecdsa_verify(message, r, s, x, y));
+        if (!FCL_ecdsa.ecdsa_verify(message, r, s, x, y)) {
+            return "";
+        }
+
+        output = abi.encode(true);
     }
 }
diff --git a/modules/passkey/test/verifiers/FCLP256Verifier.spec.ts b/modules/passkey/test/verifiers/FCLP256Verifier.spec.ts
@@ -12,28 +12,27 @@ describe('FCLP256Verifier', function () {
 
     async function verifySignature(message: BytesLike, r: BigNumberish, s: BigNumberish, x: BigNumberish, y: BigNumberish) {
       const coder = ethers.AbiCoder.defaultAbiCoder()
-      const [success] = coder.decode(
-        ['bool'],
-        await verifier.fallback!.staticCall({
-          data: coder.encode(['bytes32', 'uint256', 'uint256', 'uint256', 'uint256'], [message, r, s, x, y]),
-        }),
-      )
-      return success
+      return await verifier.fallback!.staticCall({
+        data: coder.encode(['bytes32', 'uint256', 'uint256', 'uint256', 'uint256'], [message, r, s, x, y]),
+      })
     }
 
     const account = new Account()
 
     return { verifier, verifySignature, account }
   })
 
+  const SUCCESS = `0x${'00'.repeat(31)}01`
+  const FAILURE = '0x'
+
   it('Should return 1 on valid signature', async function () {
     const { verifySignature, account } = await setupTests()
 
     const message = ethers.id('hello world')
     const { r, s } = account.sign(message)
     const { x, y } = account.publicKey
 
-    expect(await verifySignature(message, r, s, x, y)).to.be.true
+    expect(await verifySignature(message, r, s, x, y)).to.equal(SUCCESS)
   })
 
   it('Should ignore signature malleability', async function () {
@@ -43,18 +42,45 @@ describe('FCLP256Verifier', function () {
     const { r, highS } = account.sign(message)
     const { x, y } = account.publicKey
 
-    expect(await verifySignature(message, r, highS, x, y)).to.be.true
+    expect(await verifySignature(message, r, highS, x, y)).to.equal(SUCCESS)
+  })
+
+  it('Should return empty on unverified signature', async function () {
+    const { verifySignature, account } = await setupTests()
+
+    const { x, y } = account.publicKey
+
+    expect(await verifySignature(ethers.ZeroHash, 1, 2, x, y)).to.equal(FAILURE)
+  })
+
+  it('Should return empty on invalid signature parameters', async function () {
+    const { verifySignature, account } = await setupTests()
+
+    const message = ethers.id('hello world')
+    const { r, s } = account.sign(message)
+    const { x, y } = account.publicKey
+
+    // `r` and `s` must be in the range `[1, n)`, where `n` is the order of the curve.
+    expect(await verifySignature(message, 0, s, x, y)).to.equal(FAILURE)
+    expect(await verifySignature(message, r, 0, x, y)).to.equal(FAILURE)
+    expect(await verifySignature(message, ethers.MaxUint256, s, x, y)).to.equal(FAILURE)
+    expect(await verifySignature(message, r, ethers.MaxUint256, x, y)).to.equal(FAILURE)
   })
 
-  it('Should return 0 on invalid signature', async function () {
-    const { verifySignature } = await setupTests()
+  it('Should return empty on invalid public key', async function () {
+    const { verifySignature, account } = await setupTests()
+
+    const message = ethers.id('hello world')
+    const { r, s } = account.sign(message)
+    const { x, y } = account.publicKey
 
-    expect(await verifySignature(ethers.ZeroHash, 1, 2, 3, 4)).to.be.false
+    expect(await verifySignature(message, r, s, 0, y)).to.equal(FAILURE)
+    expect(await verifySignature(message, r, s, x, 0)).to.equal(FAILURE)
   })
 
-  it('Should return 0 on invalid input', async function () {
+  it('Should return empty on invalid input', async function () {
     const { verifier } = await setupTests()
 
-    expect(await verifier.fallback!.staticCall({ data: ethers.hexlify(ethers.toUtf8Bytes('invalid input')) })).to.equal(ethers.ZeroHash)
+    expect(await verifier.fallback!.staticCall({ data: ethers.hexlify(ethers.toUtf8Bytes('invalid input')) })).to.equal(FAILURE)
   })
 })