Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add comment in _checkSignaturesLength regarding dynamic signatures #471

Merged
merged 12 commits into from
Aug 27, 2024
13 changes: 12 additions & 1 deletion modules/4337/contracts/Safe4337Module.sol
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,20 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
}

/**
* @dev Checks if the signatures length is correct and does not contain additional bytes. The function does not
* @notice Checks if the signatures length is correct and does not contain additional bytes. The function does not
* check the integrity of the signature encoding, as this is expected to be checked by the {Safe} implementation
* of {checkSignatures}.
* @dev Safe account has two types of signatures: EOA and Smart Contract signatures. While the EOA signature is
* fixed in size, the Smart Contract signature can be of arbitrary length. Safe encodes the Smart Contract
* signature length in the signature data. A malicious bundler can pad additional bytes to the `signature` data,
akshay-ap marked this conversation as resolved.
Show resolved Hide resolved
* causing the account to pay more gas than needed for user operation validation and reach the
* verificationGasLimit if the verifier does not implement appropriate length checks. `_checkSignaturesLength`
* function checks for presence of any padded bytes to the `signature` data. However, there is an
* edge case that `_checkSignaturesLength` function cannot detect. Since, the `signature` field in UserOp is not
* part of the UserOp hash a malicious bundler can manipulate the field storing the signature length and pad
* additional bytes to the dynamic part of the signatures which will make `_checkSignaturesLength` to return true.
* In such cases, it is the responsibility of the signature verifier to check for additional padded bytes to the
* signatures data.
akshay-ap marked this conversation as resolved.
Show resolved Hide resolved
* @param signatures Signatures data.
* @param threshold Signer threshold for the Safe account.
* @return isValid True if length check passes, false otherwise.
Expand Down
Loading