Potential Memory Safety Issue in `getConfiguration` Function

[hats-bug-reporter[bot]]

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0xa4dd5c3c72d9afdeb62dd7d25754bcb4b0b27e420812a05f3a1d3c9d331e730e Severity: medium

Description: Description\ The getConfiguration function in the provided smart contract is designed to return the x coordinate, y coordinate, and P-256 verifiers used for ECDSA signature validation. The function directly accesses calldata using hardcoded offsets to extract these values. While this approach is flexible, it introduces potential memory safety issues due to out-of-bounds reads if the calldata size is not as expected.

Attack Scenario\ An attacker or an incorrect caller can send calldata that is smaller than expected. When the getConfiguration function tries to access the last 86, 54, and 22 bytes of the calldata, it may read out-of-bounds memory, leading to undefined behavior or reverting the transaction.

Attachments

1. Proof of Concept (PoC) File

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

Recommendation

To resolve this issue, the getConfiguration function should include safety checks to ensure the calldata size is sufficient. Additionally, the function should dynamically calculate offsets and handle the verifiers struct correctly.

[nlordell]

There is are explicit length check that prevents out of bounds reads:

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol#L96-L98

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol#L101-L103

And unit tests that verify this behaviour.

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

I will mark this submission as invalid unless there is a PoC an example of out of bound reads or other memory safety issues.

[Jelev123]

The primary issue lies in the fact that while length checks (eq(mload(0x20), 0x60), eq(returndatasize(), 0xa0)) are in place, they do not fully ensure the integrity and structure of the calldata being accessed. Specifically:

Insufficient Length Handling: If calldata is shorter than expected, accessing calldataload(sub(calldatasize(), 86)) can lead to out-of-bounds reads. Malformed Data: Even if the length checks pass, the data within calldata might not be correctly structured, leading to potential memory safety issues when interpreting it.

To enhance the robustness of the function, consider implementing additional checks within the function to ensure that:

The calldata length is not only as expected but also structured correctly. The memory accesses are safe and within bounds. For instance, validating the structure of calldata before accessing it could prevent these issues.

[nlordell]

I realised that i responded by referencing the incorrect code. We will re-evaluate and get back to you.

[nlordell]

The code you reference is the SafeWebAuthnSignerSingleton implementation. This code is documented to only work correctly with a SafeWebAuthnSignerProxy. Both these contracts are deployed by the SafeWebAuthnSignerFactory (there is no direct deployment of a SafeWebAuthnSignerSingleton), so the code of both the proxy and singleton contracts are known.

Documentation here:

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/SafeWebAuthnSignerSingleton.sol#L9-L12

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/SafeWebAuthnSignerSingleton.sol#L25-L27

And the proxy implementation:

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/SafeWebAuthnSignerProxy.sol#L9-L11

So, given a singleton that is always called by the proxy, there should be no issue as the required fields are appended to calldata.

Now, the issue you described would be an issue if:

• One of the contracts in the repository can be "tricked" into calling the singleton without it being beind a proxy (I don't think this is the case)
• The proxy can be tricked to not correctly appending the calldata.

For these reasons I do not think this is a medium issue.

That being said, there is some prior art to validating calldata length for ERC-2771 trusted forwarder setups (for example, OpenZeppelin contracts) so it could potentially be a low finding. That being said, I do still believe this is invalid (we are evaluating it internally at the moment, so we don't have a definitive answer). Do you potentially have some more details as to how you expect this to be leveraged by an attacker?

[Jelev123]

Thank you for your detailed feedback. I appreciate the considerations around the intended usage patterns and the deployment through the SafeWebAuthnSignerFactory. However, I would like to address the following points:

Potential Misuse Outside Expected Context: Although the singleton is designed to be called only by the proxy, smart contract interactions can sometimes be unpredictable. There is a possibility that an attacker or an unexpected contract interaction could call the singleton directly, bypassing the proxy. Ensuring that getConfiguration validates calldata size would mitigate this risk.

Defensive Programming: Defensive programming is crucial in smart contract development due to the immutable and public nature of deployed contracts. Implementing calldata size checks enhances the security and robustness of the contract, preventing potential vulnerabilities from being exploited, especially in unforeseen future interactions.

Calldata Size Validation: As seen in ERC-2771 trusted forwarder setups, validating calldata length is a best practice. Applying similar validation here would safeguard against incorrect calldata and out-of-bounds reads, ensuring that the contract operates securely even if the proxy or another caller does not behave as expected.

Future Proofing: Adding safety checks now ensures that the contract remains secure as the system evolves. Future modifications or integrations could introduce new interactions, and having these checks in place ensures that the singleton contract can handle unexpected calldata sizes gracefully.

Reduced Attack Surface: By including calldata size validation, the attack surface is reduced. Even if the proxy contract or another mechanism is compromised, the singleton contract will still be protected from out-of-bounds reads, maintaining the system's integrity.

Example Scenario: An attacker might find a way to call the singleton contract directly, perhaps due to a bug in another part of the system. By sending calldata that is smaller than expected, they could trigger out-of-bounds reads, leading to undefined behavior or reverting transactions, compromising the system's security.

While I understand that the proxy pattern reduces deployment gas costs, ensuring security through additional validation is a prudent trade-off given the critical nature of the system. I hope this clarifies the potential risks and the benefits of implementing these safety checks.

Thank you for considering this perspective. I look forward to your thoughts on this matter.

[nlordell]

I understand these points, and while they have some general truth to them, I don't see how they apply to our particular set of contracts. In particular, my point is that I don't think there is a possible "unexpected contract interaction" that causes this to not be called.

Note that while OpenZeppelin trusted forwarder adds checks, the Safe contracts do not. I am arguing that, like in the case of the Safe contracts, the extra defensive behaviour is not required.

Can you provide a concrete attack scenario for this that would justify your reported severity ("medium") or even a "low" severity? The scope specifically states:

Low

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

Medium

Issues that lead to an economic loss but do not lead to direct loss of on-chain assets. Examples are:

• Gas griefing attacks (make users overpay for gas)
• Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.
• Short-term freezing of user funds.

I can not think of a case where this would fall under these categories.

[Jelev123]

Unexpected Direct Interaction: While the contract is designed to be called only through the proxy, there is a potential risk of direct interaction. If an attacker finds a way to interact directly with the singleton contract, bypassing the proxy, they could send malformed calldata. This could happen due to a misconfiguration, bug in the deployment script, or an unforeseen interaction in a complex contract system.

Malicious Caller: An attacker could deploy a malicious contract that directly interacts with the singleton contract. By sending calldata shorter than expected, they could cause out-of-bounds reads, resulting in transaction reverts. This could be used as a denial-of-service attack, disrupting the contract’s functionality.

Justification for Defensive Check: Robustness and Future-proofing: Including the bounds check ensures that the contract is robust against unexpected or malicious interactions. Even if the current setup ensures the proxy always appends the correct data, future changes or extensions to the contract system could introduce new interactions. A defensive check makes the contract resilient to such changes.

Improved Error Handling: Providing explicit checks and clear error messages improves the developer experience and makes the contract easier to debug and maintain. Explicit bounds checks help identify issues quickly and provide clear feedback on the nature of the error.

Concrete Example: Imagine an attacker deploys a contract that directly calls getConfiguration with insufficient calldata. Here’s how the scenario might unfold:

contract Attacker {
    function attack(address target) external {
        bytes memory malformedData = new bytes(10); // Insufficient length
        target.call(malformedData);
    }
}

In this scenario, the getConfiguration function attempts to read out-of-bounds calldata, causing a revert. This could disrupt any legitimate interactions with the singleton contract, causing a denial-of-service.

Proposed Solution: Adding a bounds check to ensure the calldata is of the expected length would mitigate this risk:

function getConfiguration() public pure returns (uint256 x, uint256 y, P256.Verifiers verifiers) {
    require(calldatasize() >= 86, "Calldata too short");

    assembly ("memory-safe") {
        x := calldataload(sub(calldatasize(), 86))
        y := calldataload(sub(calldatasize(), 54))
        verifiers := shr(80, calldataload(sub(calldatasize(), 22)))
    }
}

Severity Justification: Medium Severity:

Denial-of-service Attack: An attacker could exploit this vulnerability to cause denial-of-service, making essential contract functionality temporarily inaccessible. While this might not lead to direct loss of on-chain assets, it disrupts contract operations, aligning with the medium severity criteria.

Low Severity:

Unexpected Behavior: The behavior of the contract would differ from the intended behavior in case of unexpected or malicious interactions. While no funds are at immediate risk, the robustness and reliability of the contract are compromised.

[nlordell]

In your concrete example, an attacker is calling the SafeWebAuthnSignerSigleton directly and just getting invalid data returned. The SafeWebAuthnSignerSigleton does not have any privileges or funds, and to me, this does not constitute a denial of service. There is no essential functionality that is temporarily unavailable (WebAuthn users can continue to use the system and are unaffected by this bad call).

the getConfiguration function attempts to read out-of-bounds calldata, causing a revert

I do not think this is true. Reading out of calldatasize bounds will read 0s for the bytes that are out of bounds. I just tested this in Remix and the function did not revert.

[Jelev123]

While out-of-bounds reads return zeros, this behavior might lead to incorrect data being processed without immediate visibility into the underlying issue. Ensuring the data is of the expected size helps maintain functional correctness.

While the risk of a denial of service due to out-of-bounds reads is mitigated by Solidity’s behavior of returning zeros, adding a bounds check still provides significant benefits in terms of functional correctness, robustness, and maintainability. I believe these benefits justify the inclusion of the check.

Thank you for considering this updated perspective. I look forward to your thoughts on this matter.

[nlordell]

In the scenario you linked, incorrect data is only given to the attacker, and not users of the contracts, so I still question the validity of this submission.