Potential Out-of-Bounds Access in `checkAuthenticatorFlags` Function

[hats-bug-reporter[bot]]

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

Description: Description\ The checkAuthenticatorFlags function in the provided smart contract checks if certain flags are set in the authenticatorData array. However, the function does not currently perform bounds checking to ensure that the authenticatorData array is long enough to access the 33rd byte (index 32). This oversight can lead to out-of-bounds access, causing the transaction to revert unexpectedly or introducing potential security vulnerabilities.

Attack Scenario\ An attacker or incorrect caller provides authenticatorData that is shorter than 33 bytes. When the checkAuthenticatorFlags function attempts to access authenticatorData[32], the transaction reverts due to out-of-bounds access. This could be exploited to cause denial of service or disrupt contract functionality.

A caller provides malformed authenticatorData that is exactly 32 bytes long. The function tries to read the 33rd byte to check the flags, resulting in a revert. This scenario might not be an intentional attack but demonstrates the lack of robustness in handling unexpected input sizes.

Attachments

1. Proof of Concept (PoC) File

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

Recommendation

To resolve this issue, the checkAuthenticatorFlags function should include bounds checking to ensure the authenticatorData array is long enough to access the required byte. Here is the revised implementation:

 function checkAuthenticatorFlags(
        bytes calldata authenticatorData,
        AuthenticatorFlags authenticatorFlags
    ) internal pure returns (bool success) {
        // Ensure that authenticatorData is long enough to contain the flags byte
        require(authenticatorData.length > 32, "Authenticator data is too short");

        // Perform bitwise AND to check if all required flags are set
        success = (authenticatorData[32] & AuthenticatorFlags.unwrap(authenticatorFlags)) == AuthenticatorFlags.unwrap(authenticatorFlags);
    }

[nlordell]

Note that the Solidity compile will panic with a specific error code when attempting to access the bytes array out of bounds. This functionally equivalent to a revert, but with different returndata.

8. 0x32: If you access an array, bytesN or an array slice at an out-of-bounds or negative index (i.e. x[i] where i >= x.length or i < 0).

As such, I believe this submission to be invalid.

[Jelev123]

Thank you for your feedback. I appreciate your reference to the Solidity compiler's behavior when accessing arrays out of bounds. While it’s true that the Solidity compiler will panic with a specific error code (0x32) in such cases, I would like to emphasize the following points that underscore the importance of explicit bounds checking in the checkAuthenticatorFlags function:

Improved Code Readability and Clarity: Explicitly including bounds checking in the code makes the intent and behavior of the function clearer to anyone reading the code. Developers can immediately understand that the function checks the length of authenticatorData before accessing its 33rd byte, reducing cognitive load and potential misunderstandings about the function's safety.

Enhanced Robustness and Maintenance: By incorporating explicit bounds checking, we ensure that the contract handles all input cases gracefully. This makes the contract more robust and easier to maintain, especially for future developers who may not be aware of the specific error codes and behaviors of the Solidity compiler.

Clearer Error Messaging: The proposed bounds check provides a clear and specific error message ("Authenticator data is too short"), which can be extremely valuable during debugging and development. This message immediately indicates the nature of the issue, whereas the generic panic error code 0x32 requires additional context to interpret.

Defensive Programming Best Practices: In the context of smart contract security, defensive programming is a best practice. By explicitly checking bounds, we reduce the risk of future vulnerabilities that might arise from changes in the contract or its interactions with other contracts.

User and Developer Experience: Providing explicit checks and clear error messages improves the user and developer experience. Users interacting with the contract will benefit from more informative error messages, leading to quicker resolution of issues and smoother operation.

In light of these points, I believe that adding explicit bounds checking is a worthwhile enhancement. It does not only prevent potential out-of-bounds access but also aligns with best practices for writing secure, maintainable, and user-friendly smart contracts.

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

[nlordell]

Thank you for the comprehensive discussion:

Improved Code Readability and Clarity

I don't think excessive checks would add clarity to the code. This would be the same as arguing that all math operations in Solidity 0.8 should still use checked math to improve readability instead of relying on the compiler which I don't agree with.

Enhanced Robustness and Maintenance

Because of the compiler insurances, it is already the case that all conditions are handled by the code.

Clearer Error Messaging

I would argue that adding a non-standard custom error makes the code harder to integrate with, instead of using error that is standardized and built into the compiler.

---

In general, I don't think this is a valid submission (and definitely not a medium severity) submission. While I understand your points, I disagree with them and would keep this as invalid.

[Jelev123]

Ok i understand that . It`s your call. Thank you for discussion.