Skip to content

fix(1271): separate policyData and signature#177

Open
highskore wants to merge 2 commits into
mainfrom
fix/1271-data
Open

fix(1271): separate policyData and signature#177
highskore wants to merge 2 commits into
mainfrom
fix/1271-data

Conversation

@highskore

@highskore highskore commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

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.

Copilot AI left a comment

Copy link
Copy Markdown

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.

Comment thread contracts/SmartSession.sol
Comment thread contracts/SmartSession.sol Outdated
Comment thread contracts/SmartSession.sol
Comment thread contracts/SmartSession.sol
Comment thread contracts/SmartSession.sol Outdated
Comment thread contracts/SmartSession.sol
Comment thread contracts/SmartSession.sol Outdated
requestSender: sender,
hash: hash,
signature: signature,
signature: signature[64:remainingSignatureOffset], // extract policy data from the signature

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@highskore

highskore commented Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

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.

4 participants