Skip to content

Conversation

@highskore
Copy link
Contributor

@highskore highskore commented Oct 15, 2025

There's a bug in 1271 where we pass the whole signature to both the policy and validator, meaning that both the validator and the policy would need to know how to destructure the same data, which obviously doesn't work with OwnableValidator etc.

This PR fixes this by extracting policyData from the signature before passing it to the policy and then passes the signature data to the stateless validator.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Separates concatenated policy data from the validator signature in ERC-1271 flow by introducing an explicit length-delimited policyData segment inside the aggregated signature format. Updates test construction of composite signatures and parsing logic in SmartSession to extract policy data and remaining validator signature.

  • Introduces new signature layout: permissionId | policyDataLength | policyData | validatorSignature
  • Adjusts isValidSignatureWithSender to parse and dispatch policyData and validator signature separately
  • Refactors tests to build signatures following the new structured layout

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
test/unit/base/ERC1271Base.t.sol Updates test helper to construct new composite signature with embedded policyData length and blob.
contracts/SmartSession.sol Parses new signature structure: extracts policyData length, slices policyData for ERC1271 policy checks, and separates remaining validator signature.
Comments suppressed due to low confidence (1)

test/unit/base/ERC1271Base.t.sol:1

  • [nitpick] Hard-coded length literal (uint256(32)) and an opaque bytes32 constant reduce clarity of the new signature structure; consider introducing named constants or helper builders (e.g., mockPolicyData(), POLICY_DATA_LENGTH) to document intent and avoid duplication across tests.
import "../../Base.t.sol";

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

requestSender: sender,
hash: hash,
signature: signature,
signature: signature[64:remainingSignatureOffset], // extract policy data from the signature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@highskore
Copy link
Contributor Author

highskore commented Oct 15, 2025

added some bounds checks & consts @zeroknots

@highskore highskore requested a review from filmakarov October 22, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants