Feature: Stay in calldata for validation, remove the need for an external library

[mmv08]

We've been working on a P256 signer at Safe, and I used this repo as a reference implementation. I wasn't too fond of using an external library to validate signatures; that's too gas-heavy. The need for an external library was caused by the library using calldata for dynamic types everywhere. This wasn't possible because the public key was expected as an array, but it was stored in the contract's state storage; that's why we suggested a change in the freshcryptolib: rdubois-crypto/FreshCryptoLib#48

This PR:

• Removes an external wrapper library for webauthn signature checker
• Parse the signature using @k06a's struct bind trick
• Updates FCL/forge-std dependencies

Gas report:

| contracts/P256Signer.sol:P256Signer contract |                 |       |        |        |         |
|----------------------------------------------|-----------------|-------|--------|--------|---------|
| Deployment Cost                              | Deployment Size |       |        |        |         |
| 1677260                                      | 8312            |       |        |        |         |
| Function Name                                | min             | avg   | median | max    | # calls |
| initialize                                   | 449             | 50429 | 66756  | 66756  | 8       |
| initialized                                  | 333             | 1333  | 1333   | 2333   | 2       |
| isValidSignature                             | 930             | 75206 | 9597   | 215093 | 3       |
| x                                            | 285             | 1285  | 1285   | 2285   | 2       |
| y                                            | 306             | 1306  | 1306   | 2306   | 2       |

Gas report from main:

| contracts/P256Signer.sol:P256Signer contract |                 |       |        |        |         |
|----------------------------------------------|-----------------|-------|--------|--------|---------|
| Deployment Cost                              | Deployment Size |       |        |        |         |
| 419560                                       | 2030            |       |        |        |         |
| Function Name                                | min             | avg   | median | max    | # calls |
| initialize                                   | 449             | 50429 | 66756  | 66756  | 8       |
| initialized                                  | 333             | 1333  | 1333   | 2333   | 2       |
| isValidSignature                             | 1270            | 80157 | 16798  | 222404 | 3       |
| x                                            | 285             | 1285  | 1285   | 2285   | 2       |
| y                                            | 306             | 1306  | 1306   | 2306   | 2       |

Seems like it went from 222404 max to 215093.

[nlordell]

Do you know if this generates code for bounds checks on the dynamic calldata fields from the structure?

As in, what happens if you pass in malformed calldata to this method?

[mmv08]

So, a fuzz test verifies that the method fails in case of an invalid signature (random bytes array). I commented on it above because I had to adjust the expected revert message. The trace suggested that it failed with panic: array out-of-bounds access (0x32), so I bet it generates code for bound checks. Will verify.

[cristovaoth]

Had crossed my mind. I guess it works while everything still points within (calldata is large enough)?

[mmv08]

So this is the IR of the above code, including the call to checkSignature:

                        let var__hash_97 := expr_100
                        /// @src 0:2485:2526  "EncodedSignature calldata parsedSignature"
                        let var_parsedSignature_104_offset
                        let zero_t_struct$_EncodedSignature_$88_calldata_ptr_12_offset := zero_value_for_split_t_struct$_EncodedSignature_$88_calldata_ptr()
                        var_parsedSignature_104_offset := zero_t_struct$_EncodedSignature_$88_calldata_ptr_12_offset
                        /// @src 0:2536:2605  "assembly {..."
                        {
                            var_parsedSignature_104_offset := var__signature_93_offset
                        }
                        /// @src 0:2628:2640  "FCL_WebAuthn"
                        let expr_109_address := linkersymbol("lib/FreshCryptoLib/solidity/src/FCL_Webauthn.sol:FCL_WebAuthn")
                        /// @src 0:2669:2684  "parsedSignature"
                        let _13_offset := var_parsedSignature_104_offset
                        let expr_111_offset := _13_offset
                        /// @src 0:2669:2702  "parsedSignature.authenticatorData"
                        let _14 := add(expr_111_offset, 0)
                        let expr_112_offset, expr_112_length := access_calldata_tail_t_bytes_calldata_ptr(expr_111_offset, _14)
                        /// @src 0:2716:2720  "0x01"
                        let expr_113 := 0x01
                        /// @src 0:2734:2749  "parsedSignature"
                        let _15_offset := var_parsedSignature_104_offset
                        let expr_114_offset := _15_offset
                        /// @src 0:2734:2760  "parsedSignature.clientData"
                        let _16 := add(expr_114_offset, 32)
                        let expr_115_offset, expr_115_length := access_calldata_tail_t_bytes_calldata_ptr(expr_114_offset, _16)
                        /// @src 0:2774:2779  "_hash"
                        let _17 := var__hash_97
                        let expr_116 := _17
                        /// @src 0:2793:2808  "parsedSignature"
                        let _18_offset := var_parsedSignature_104_offset
                        let expr_117_offset := _18_offset
                        /// @src 0:2793:2824  "parsedSignature.challengeOffset"
                        let _19 := add(expr_117_offset, 64)
                        let expr_118 := read_from_calldatat_uint256(_19)
                        /// @src 0:2838:2853  "parsedSignature"
                        let _20_offset := var_parsedSignature_104_offset
                        let expr_119_offset := _20_offset
                        /// @src 0:2838:2856  "parsedSignature.rs"
                        let _21 := add(expr_119_offset, 96)
                        let expr_120_offset := _21
                        /// @src 0:2870:2871  "x"
                        let _22 := read_from_storage_split_offset_0_t_uint256(0x01)
                        let expr_121 := _22
                        /// @src 0:2885:2886  "y"
                        let _23 := read_from_storage_split_offset_0_t_uint256(0x02)
                        let expr_122 := _23
                        /// @src 0:2628:2896  "FCL_WebAuthn.checkSignature(..."
                        let _24 := convert_t_rational_1_by_1_to_t_bytes1(expr_113)
                        let expr_123 := fun_checkSignature_434(expr_112_offset, expr_112_length, _24, expr_115_offset, expr_115_length, expr_116, expr_118, expr_120_offset, expr_121, expr_122)

So, if I'm reading this right, it calculates the offsets (and length for dynamic types) based on the struct layout and passes them to check the checkSIgnatures function. I think @cristovaoth is right about everything working if the calldata is large enough.

[nlordell]

It looks like for values, it uses read_from_calldatat_uint256 to read values from calldata and for dynamic bytes, it uses access_calldata_tail_t_bytes_calldata_ptr. What do those YUL function look like?

[mmv08]

                    function access_calldata_tail_t_bytes_calldata_ptr(base_ref, ptr_to_tail) -> addr, length {
                        let rel_offset_of_tail := calldataload(ptr_to_tail)
                        if iszero(slt(rel_offset_of_tail, sub(sub(calldatasize(), base_ref), sub(0x20, 1)))) { revert_error_356d538aaf70fba12156cc466564b792649f8f3befb07b071c91142253e175ad() }
                        addr := add(base_ref, rel_offset_of_tail)

                        length := calldataload(addr)
                        if gt(length, 0xffffffffffffffff) { revert_error_1e55d03107e9c4f1b5e21c76a16fba166a461117ab153bcce65e6a4ea8e5fc8a() }
                        addr := add(addr, 32)
                        if sgt(addr, sub(calldatasize(), mul(length, 0x01))) { revert_error_977805620ff29572292dee35f70b0f3f3f73d3fdd0e9f4d7a901c2e43ab18a2e() }

                    }

and

                    function read_from_calldatat_uint256(ptr) -> returnValue {

                        let value := calldataload(ptr)
                        validator_revert_t_uint256(value)

                        returnValue :=

                        value

                    }
                    
                   function validator_revert_t_uint256(value) {
                        if iszero(eq(value, cleanup_t_uint256(value))) { revert(0, 0) }
                    }
                    
                   function cleanup_t_uint256(value) -> cleaned {
                        cleaned := value
                    }

[nlordell]

I think

Much confidence :P

[Kelvyne]

I think calldata variables are represented as offsets only when they are of dynamic size. If this is the case, I'm not sure the comment explains this well :)
I interpret the following assembly as : it "aligns" signaturePointer with the calldata`. Am I wrong here ?

[mmv08]

I think before accessing the variable and copying it to the stack with calldataload(offset), it represents it as an offset. This can be seen in the Yul output I attached to another discussion

[mmv08]

The tests seem to fail because the infura project ID (or some other credential) is not shared with a fork. I can confirm the tests work locally

[Kelvyne]

Thank you for the improvement!
The CI not passing is indeed "normal". I also checked that everything works locally.

LGTM I'll still let you resolve the last conversation before merging!

[Kelvyne]

I think calldata variables are represented as offsets only when they are of dynamic size. If this is the case, I'm not sure the comment explains this well :)
I interpret the following assembly as : it "aligns" signaturePointer with the calldata`. Am I wrong here ?

[mmv08]

@Kelvyne I spent more time on the calldata decoding and found out that it doesn't generate bound checks, and it's possible to mess up offsets for signature parameters. You can find my notes here: https://lamps-judge-y35.craft.me/7Ajz24lN7iLclG

Still, at first sight, this doesn't posses any security risk since the hash for the validation is passed separately, and any mess in signature parameters should cause the validation to fail. I'm letting you know in case you have more thoughts.

[mmv08]

I checked and solidity's compiler generated code for structs doesn't include bound checks either: https://sandbox.tenderly.co/mikhail-gnosis/thousands-eggplant

[Kelvyne]

@Kelvyne I spent more time on the calldata decoding and found out that it doesn't generate bound checks, and it's possible to mess up offsets for signature parameters. You can find my notes here: https://lamps-judge-y35.craft.me/7Ajz24lN7iLclG

Still, at first sight, this doesn't posses any security risk since the hash for the validation is passed separately, and any mess in signature parameters should cause the validation to fail. I'm letting you know in case you have more thoughts.

Hey, thanks for the heads up.
I also believe that it should not cause problems in our case, but it could be wiser to revert this PR. We'll discuss it internally!

[mmv08]

@Kelvyne I spent more time on the calldata decoding and found out that it doesn't generate bound checks, and it's possible to mess up offsets for signature parameters. You can find my notes here: https://lamps-judge-y35.craft.me/7Ajz24lN7iLclG
Still, at first sight, this doesn't posses any security risk since the hash for the validation is passed separately, and any mess in signature parameters should cause the validation to fail. I'm letting you know in case you have more thoughts.

Hey, thanks for the heads up. I also believe that it should not cause problems in our case, but it could be wiser to revert this PR. We'll discuss it internally!

We at Safe will continue using it as is