Check `staticcall` Result From SHA-256 Precompile

[nlordell]

Fixes #14

This PR changes the _sha256 implementation to check the result from the static call. There is a very subtle bug with not checking, where, for very large inputs, you would be able to get the precompile to revert but have the function finish executing successfully (and use whatever is in the scratch space as the digest).

Note that we do not check the length of the returndata. This is intentional and the same thing that the Solidity compiler does for the sha256 builtin.

[mmv08]

do I get it right that the precompile would check if there’s enough gas supplied and revert if there's not enough?

[nlordell]

do I get it right that the precompile would check if there’s enough gas supplied and revert if there's not enough?

Correct. Basically, you would call the precompile with some gas, and the precompile computes the gas needed based on the input size, and reverts if it isn't enough.

Ethereum has the 1/64ths rule, so for very large inputs, you can have a 1/64th of the remaining gas that is still enough to finish the function successfully despite the precompile reverting.

[nlordell]

For fun, if you want to see an example of this yielding unexpected results, apply this patch:

diff --git a/modules/passkey/contracts/libraries/WebAuthn.sol b/modules/passkey/contracts/libraries/WebAuthn.sol
index 28086ae..6e4e7a5 100644
--- a/modules/passkey/contracts/libraries/WebAuthn.sol
+++ b/modules/passkey/contracts/libraries/WebAuthn.sol
@@ -348,7 +348,7 @@ library WebAuthn {
             // its input bytes on success. Note that this is similar to the code generated by the
             // Solidity compiler for the `sha256` built-in.
             if iszero(staticcall(gas(), 0x0002, add(input, 0x20), mload(input), 0, 32)) {
-                revert(0, 0)
+                // revert(0, 0)
             }
             digest := mload(0)
         }
diff --git a/modules/passkey/test/libraries/WebAuthn.spec.ts b/modules/passkey/test/libraries/WebAuthn.spec.ts
index bb7e9f9..e7061a8 100644
--- a/modules/passkey/test/libraries/WebAuthn.spec.ts
+++ b/modules/passkey/test/libraries/WebAuthn.spec.ts
@@ -151,7 +151,7 @@ describe('WebAuthn Library', () => {
       expect(await webAuthnLib.encodeSigningMessage(challenge, authenticatorData, `"origin":"http://safe.global"`)).to.equal(message)
     })

-    it('Should revert if SHA-256 precompile reverts', async () => {
+    it.only('Should revert if SHA-256 precompile reverts', async () => {
       const { webAuthnLib } = await setupTests()

       // This test is a bit tricky - the SHA-256 precompile can be made to revert by calling it
@@ -161,7 +161,8 @@ describe('WebAuthn Library', () => {
       // a large enough client data and exact gas limits to make this happen is a bit annoying, so
       // lets hope for no gas schedule changes :fingers_crossed:.
       const longClientDataFields = `"long":"${'a'.repeat(100000)}"`
-      await expect(webAuthnLib.encodeSigningMessage(ethers.ZeroHash, '0x', longClientDataFields, { gasLimit: 1701001 })).to.be.reverted
+      console.log(await webAuthnLib.encodeSigningMessage(ethers.ZeroHash, '0x', longClientDataFields))
+      console.log(await webAuthnLib.encodeSigningMessage(ethers.ZeroHash, '0x', longClientDataFields, { gasLimit: 1701001 }))
     })
   })

And run the tests to get this output:

  WebAuthn Library
    encodeSigningMessage
0xd124daa65615e0447630ec385524913b92abf6adb9d14a384d5060c3c36f6357
0x0000000000000000000000000000000000000000000000000000000000000041
      ✔ Should revert if SHA-256 precompile reverts (460ms)

Note that the:

1. function does not revert
2. returns invalid encoded client data

[mmv08]

Yeah, thats how I understood it, I just wanted to make sure my understanding was correct. Thanks. PR approved.

[nlordell]

no one requested my review but I approve

I requested in Slack :smile: