Skip to content

fix: use wallet-substituted account in returned spend permission#349

Open
Snowmanzx wants to merge 1 commit into
base:masterfrom
Snowmanzx:fix/spend-permission-substituted-account
Open

fix: use wallet-substituted account in returned spend permission#349
Snowmanzx wants to merge 1 commit into
base:masterfrom
Snowmanzx:fix/spend-permission-substituted-account

Conversation

@Snowmanzx

Copy link
Copy Markdown

requestSpendPermission sends mutableData: { fields: ['message.account'] } so the wallet may replace the account (for example swapping an EOA for a smart wallet). It computes permissionHash from the wallet-returned signedData.message, but built the returned SpendPermission.permission from the original typedData.message.

When the wallet actually substitutes the account the two disagree: permissionHash is over the new account while permission.account still holds the original. Anything downstream that reads result.permission.account (display, storage, prepareSpendCallData, fetchPermissions) then sees the wrong address.

Carry the signed message through a variable and use it for both the hash and the returned permission. The eth_signTypedData_v4 branch has no substitution so it is unchanged.

Added a test where signedData.message.account differs from the request account and checks the returned permission matches the substituted message, not the original.

Closes #324

requestSpendPermission lets the wallet replace message.account via mutableData and computes permissionHash from the wallet-returned message, but built the returned permission object from the original pre-substitution typed data. When the wallet substituted the account, permission.account and permissionHash described different permissions. Carry the signed message through a variable and use it for both. Added a test that exercises the substitution.
@cb-heimdall

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@albertoblue87-netizen albertoblue87-netizen 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.

Hecho

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.

spend-permission: requestSpendPermission returns inconsistent SpendPermission when wallet substitutes account via mutableData

3 participants