SafeWebAuthnSharedSigner : getConfiguration() - staticcall return data is not validated

[hats-bug-reporter[bot]]

Github username: @aktech297 Twitter username: kaka Submission hash (on-chain): 0xfe342c3a2fff3902117c6ce35b177b267ae0eaa81bbb9f012b2807e601b8eb57 Severity: medium

Description: Description\

To get the configuration for the msg.sender, the function _verifySignature calls the getConfiguration.

The getConfiguration do static call to write the first two words of the return data into the scratch.

SafeWebAuthnSharedSigner.sol#L101-L108

                    // The length of the return data should be exactly 0xa0 bytes, which should
                    // always be the case for the Safe's `getStorageAt` implementation.
                    eq(returndatasize(), 0xa0),
                    // The call succeeded. We write the first two words of the return data into the
                    // scratch space, as we need to inspect them before copying the signer
                    // signer configuration to our `signer` memory pointer.
                    staticcall(gas(), account, add(getStorageAtData, 0x20), mload(getStorageAtData), 0x00, 0x40)
                )

But the return data of this static call is not validated.

Impact\ Unexpected inconsistencies in signature validation.

Also, following are the other impacts of this one.

Improper error handling when the return data validation fails can lead to vulnerabilities. For instance, if the validation logic does not revert the transaction appropriately upon receiving invalid data, it could cause downstream issues in the contract's operations.

The staticcall function might return data that isn't in the expected format. This could lead to issues in decoding or interpreting the return data, especially if the calling contract expects a specific type or structure of data.

Attachments

1. Proof of Concept (PoC) File SafeWebAuthnSharedSigner.sol#L101-L107

2. Revised Code File (Optional) Other parts of the code hanlde this correclty.. Please check in following place.

FCL_elliptic.sol#L88-L91

FCL_elliptic.sol#L489-L490

FCL_elliptic.sol#L66-L69

[aktech297]

Also, following place has the name but it checks the failure state and work.

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/SafeWebAuthnSignerFactory.sol#L79-L86

[nlordell]

The code you linked includes both validation of the return success, and that the length is exactly 0xa0 bytes (which is more than the 0x40 bytes that are being copied into the scratch space).

                    // The length of the return data should be exactly 0xa0 bytes, which should
                    // always be the case for the Safe's `getStorageAt` implementation.
                    eq(returndatasize(), 0xa0),
                    // The call succeeded. We write the first two words of the return data into the
                    // scratch space, as we need to inspect them before copying the signer
                    // signer configuration to our `signer` memory pointer.
                    staticcall(gas(), account, add(getStorageAtData, 0x20), mload(getStorageAtData), 0x00, 0x40)

Note that these expressions are part of the bool parameter of the if.

Additionally, the repository contains tests that demonstrate that it handles these cases correctly:

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/test/4337/SafeWebAuthnSharedSigner.spec.ts#L80-L105

As such, I believe this submission is invalid. If you disagree can you please provide a counter example that shows that the staticcall success is not validated?

[aktech297]

The code you linked includes both validation of the return success, and that the length is exactly 0xa0 bytes (which is more than the 0x40 bytes that are being copied into the scratch space).

                    // The length of the return data should be exactly 0xa0 bytes, which should
                    // always be the case for the Safe's `getStorageAt` implementation.
                    eq(returndatasize(), 0xa0),
                    // The call succeeded. We write the first two words of the return data into the
                    // scratch space, as we need to inspect them before copying the signer
                    // signer configuration to our `signer` memory pointer.
                    staticcall(gas(), account, add(getStorageAtData, 0x20), mload(getStorageAtData), 0x00, 0x40)

Note that these expressions are part of the bool parameter of the if.

Additionally, the repository contains tests that demonstrate that it handles these cases correctly:

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/test/4337/SafeWebAuthnSharedSigner.spec.ts#L80-L105

As such, I believe this submission is invalid. If you disagree can you please provide a counter example that shows that the staticcall success is not validated?

my submission is related to this https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol#L101-L108 and there were no if checks made.. can you pls check again ?

[nlordell]

my submission is related to this

This is the code I also referenced. In addition I included references to unit test that verify the code you are referencing that shows it is behaving as expected and verifies lengths and staticcall success.

and there were no if checks made..

The larger code in question is:

            if and(
                and(
                    // The offset of the ABI encoded bytes is 0x20, this should always be the case
                    // for standard ABI encoding of `(bytes)` tuple that `getStorageAt` returns.
                    eq(mload(0x00), 0x20),
                    // The length of the encoded bytes is exactly 0x60 bytes (i.e. 3 words, which is
                    // exactly how much we read from the Safe's storage in the `getStorageAt` call).
                    eq(mload(0x20), 0x60)
                ),
                and(
                    // The length of the return data should be exactly 0xa0 bytes, which should
                    // always be the case for the Safe's `getStorageAt` implementation.
                    eq(returndatasize(), 0xa0),
                    // The call succeeded. We write the first two words of the return data into the
                    // scratch space, as we need to inspect them before copying the signer
                    // signer configuration to our `signer` memory pointer.
                    staticcall(gas(), account, add(getStorageAtData, 0x20), mload(getStorageAtData), 0x00, 0x40)
                )
            ) {
                // Copy only the storage values from the return data to our `signer` memory address.
                // This only happens on success, so the `signer` value will be zeroed out if any of
                // the above conditions fail, indicating that no signer is configured.
                returndatacopy(signer, 0x40, 0x60)
            }

without the comments, this is:

            if and(
                and(
                    eq(mload(0x00), 0x20),
                    eq(mload(0x20), 0x60)
                ),
                and(
                    eq(returndatasize(), 0xa0),
                    staticcall(gas(), account, add(getStorageAtData, 0x20), mload(getStorageAtData), 0x00, 0x40)
                )
            ) {
                returndatacopy(signer, 0x40, 0x60)
            }

Notice the if and(and(eq(), eq()), and(eq(), staticcall())) { }. This if statement therefore checks that:

• staticcall is a 1 and thus was successful
• eq(returndatasize(), 0xa0) that the return data size is a specific length
• eq(mload(0x00), 0x20) that the first word of return data is the expected 0x20
• eq(mload(0x20), 0x60) that the second word of return data is the expected 0x60

I believe that these checks are sufficient to validate the format of the return data (or at least that is indistinguishable from valid return data). Can you provide a counter example where the data validation can be fooled?