Skip to content

Commit 80c6eb4

Browse files
joaquim-vergesjoaquim-verges
authored
Fix AccountPermissions type hash (#538)
1 parent 234408f commit 80c6eb4

File tree

7 files changed

+149
-111
lines changed

7 files changed

+149
-111
lines changed

‎contracts/extension/interface/IAccountPermissions.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,14 @@ interface IAccountPermissions {
2323
*/
2424
struct SignerPermissionRequest {
2525
address signer;
26+
uint8 isAdmin;
2627
address[] approvedTargets;
2728
uint256 nativeTokenLimitPerTransaction;
2829
uint128 permissionStartTimestamp;
2930
uint128 permissionEndTimestamp;
3031
uint128 reqValidityStartTimestamp;
3132
uint128 reqValidityEndTimestamp;
3233
bytes32 uid;
33-
uint8 isAdmin;
3434
}
3535

3636
/**

‎contracts/extension/upgradeable/AccountPermissions.sol

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
4242

4343
bytes32 private constant TYPEHASH =
4444
keccak256(
45-
"SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
45+
"SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
4646
);
4747

4848
modifier onlyAdmin() virtual {
@@ -238,6 +238,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
238238
abi.encode(
239239
TYPEHASH,
240240
_req.signer,
241+
_req.isAdmin,
241242
keccak256(abi.encodePacked(_req.approvedTargets)),
242243
_req.nativeTokenLimitPerTransaction,
243244
_req.permissionStartTimestamp,

‎src/test/smart-wallet/Account.t.sol

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ contract SimpleAccountTest is BaseTest {
5656

5757
event AccountCreated(address indexed account, address indexed accountAdmin);
5858

59-
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
59+
function _prepareSignature(IAccountPermissions.SignerPermissionRequest memory _req)
6060
internal
6161
view
62-
returns (bytes memory signature)
62+
returns (bytes32 typedDataHash)
6363
{
6464
bytes32 typehashSignerPermissionRequest = keccak256(
65-
"SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
65+
"SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
6666
);
6767
bytes32 nameHash = keccak256(bytes("Account"));
6868
bytes32 versionHash = keccak256(bytes("1"));
@@ -71,20 +71,32 @@ contract SimpleAccountTest is BaseTest {
7171
);
7272
bytes32 domainSeparator = keccak256(abi.encode(typehashEip712, nameHash, versionHash, block.chainid, sender));
7373

74-
bytes memory encodedRequest = abi.encode(
74+
bytes memory encodedRequestStart = abi.encode(
7575
typehashSignerPermissionRequest,
7676
_req.signer,
77+
_req.isAdmin,
7778
keccak256(abi.encodePacked(_req.approvedTargets)),
78-
_req.nativeTokenLimitPerTransaction,
79+
_req.nativeTokenLimitPerTransaction
80+
);
81+
82+
bytes memory encodedRequestEnd = abi.encode(
7983
_req.permissionStartTimestamp,
8084
_req.permissionEndTimestamp,
8185
_req.reqValidityStartTimestamp,
8286
_req.reqValidityEndTimestamp,
8387
_req.uid
8488
);
85-
bytes32 structHash = keccak256(encodedRequest);
86-
bytes32 typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
8789

90+
bytes32 structHash = keccak256(bytes.concat(encodedRequestStart, encodedRequestEnd));
91+
typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
92+
}
93+
94+
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
95+
internal
96+
view
97+
returns (bytes memory signature)
98+
{
99+
bytes32 typedDataHash = _prepareSignature(_req);
88100
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountAdminPKey, typedDataHash);
89101
signature = abi.encodePacked(r, s, v);
90102
}
@@ -360,14 +372,14 @@ contract SimpleAccountTest is BaseTest {
360372

361373
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
362374
accountSigner,
375+
0,
363376
approvedTargets,
364377
1 ether,
365378
0,
366379
type(uint128).max,
367380
0,
368381
type(uint128).max,
369-
uidCache,
370-
0
382+
uidCache
371383
);
372384

373385
vm.prank(accountAdmin);
@@ -398,14 +410,14 @@ contract SimpleAccountTest is BaseTest {
398410

399411
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
400412
accountSigner,
413+
0,
401414
approvedTargets,
402415
1 ether,
403416
0,
404417
type(uint128).max,
405418
0,
406419
type(uint128).max,
407-
uidCache,
408-
0
420+
uidCache
409421
);
410422

411423
vm.prank(accountAdmin);
@@ -454,14 +466,14 @@ contract SimpleAccountTest is BaseTest {
454466
approvedTargets[0] = address(numberContract);
455467
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
456468
accountSigner,
469+
0,
457470
approvedTargets,
458471
1 ether,
459472
0,
460473
type(uint128).max,
461474
0,
462475
type(uint128).max,
463-
uidCache,
464-
0
476+
uidCache
465477
);
466478

467479
vm.prank(accountAdmin);
@@ -603,14 +615,14 @@ contract SimpleAccountTest is BaseTest {
603615

604616
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
605617
accountSigner,
618+
0,
606619
approvedTargets,
607620
1 ether,
608621
0,
609622
type(uint128).max,
610623
0,
611624
type(uint128).max,
612-
uidCache,
613-
0
625+
uidCache
614626
);
615627

616628
vm.prank(accountAdmin);

‎src/test/smart-wallet/AccountVulnPOC.t.sol

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ contract SimpleAccountVulnPOCTest is BaseTest {
8080

8181
event AccountCreated(address indexed account, address indexed accountAdmin);
8282

83-
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
83+
function _prepareSignature(IAccountPermissions.SignerPermissionRequest memory _req)
8484
internal
8585
view
86-
returns (bytes memory signature)
86+
returns (bytes32 typedDataHash)
8787
{
8888
bytes32 typehashSignerPermissionRequest = keccak256(
89-
"SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
89+
"SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
9090
);
9191
bytes32 nameHash = keccak256(bytes("Account"));
9292
bytes32 versionHash = keccak256(bytes("1"));
@@ -95,20 +95,32 @@ contract SimpleAccountVulnPOCTest is BaseTest {
9595
);
9696
bytes32 domainSeparator = keccak256(abi.encode(typehashEip712, nameHash, versionHash, block.chainid, sender));
9797

98-
bytes memory encodedRequest = abi.encode(
98+
bytes memory encodedRequestStart = abi.encode(
9999
typehashSignerPermissionRequest,
100100
_req.signer,
101+
_req.isAdmin,
101102
keccak256(abi.encodePacked(_req.approvedTargets)),
102-
_req.nativeTokenLimitPerTransaction,
103+
_req.nativeTokenLimitPerTransaction
104+
);
105+
106+
bytes memory encodedRequestEnd = abi.encode(
103107
_req.permissionStartTimestamp,
104108
_req.permissionEndTimestamp,
105109
_req.reqValidityStartTimestamp,
106110
_req.reqValidityEndTimestamp,
107111
_req.uid
108112
);
109-
bytes32 structHash = keccak256(encodedRequest);
110-
bytes32 typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
111113

114+
bytes32 structHash = keccak256(bytes.concat(encodedRequestStart, encodedRequestEnd));
115+
typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
116+
}
117+
118+
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
119+
internal
120+
view
121+
returns (bytes memory signature)
122+
{
123+
bytes32 typedDataHash = _prepareSignature(_req);
112124
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountAdminPKey, typedDataHash);
113125
signature = abi.encodePacked(r, s, v);
114126
}
@@ -235,14 +247,14 @@ contract SimpleAccountVulnPOCTest is BaseTest {
235247

236248
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
237249
accountSigner,
250+
0,
238251
approvedTargets,
239252
1 ether,
240253
0,
241254
type(uint128).max,
242255
0,
243256
type(uint128).max,
244-
uidCache,
245-
0
257+
uidCache
246258
);
247259

248260
vm.prank(accountAdmin);

‎src/test/smart-wallet/DynamicAccount.t.sol

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,13 @@ contract DynamicAccountTest is BaseTest {
7272

7373
event AccountCreated(address indexed account, address indexed accountAdmin);
7474

75-
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
75+
function _prepareSignature(IAccountPermissions.SignerPermissionRequest memory _req)
7676
internal
7777
view
78-
returns (bytes memory signature)
78+
returns (bytes32 typedDataHash)
7979
{
8080
bytes32 typehashSignerPermissionRequest = keccak256(
81-
"SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
81+
"SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
8282
);
8383
bytes32 nameHash = keccak256(bytes("Account"));
8484
bytes32 versionHash = keccak256(bytes("1"));
@@ -87,20 +87,32 @@ contract DynamicAccountTest is BaseTest {
8787
);
8888
bytes32 domainSeparator = keccak256(abi.encode(typehashEip712, nameHash, versionHash, block.chainid, sender));
8989

90-
bytes memory encodedRequest = abi.encode(
90+
bytes memory encodedRequestStart = abi.encode(
9191
typehashSignerPermissionRequest,
9292
_req.signer,
93+
_req.isAdmin,
9394
keccak256(abi.encodePacked(_req.approvedTargets)),
94-
_req.nativeTokenLimitPerTransaction,
95+
_req.nativeTokenLimitPerTransaction
96+
);
97+
98+
bytes memory encodedRequestEnd = abi.encode(
9599
_req.permissionStartTimestamp,
96100
_req.permissionEndTimestamp,
97101
_req.reqValidityStartTimestamp,
98102
_req.reqValidityEndTimestamp,
99103
_req.uid
100104
);
101-
bytes32 structHash = keccak256(encodedRequest);
102-
bytes32 typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
103105

106+
bytes32 structHash = keccak256(bytes.concat(encodedRequestStart, encodedRequestEnd));
107+
typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
108+
}
109+
110+
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
111+
internal
112+
view
113+
returns (bytes memory signature)
114+
{
115+
bytes32 typedDataHash = _prepareSignature(_req);
104116
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountAdminPKey, typedDataHash);
105117
signature = abi.encodePacked(r, s, v);
106118
}
@@ -472,14 +484,14 @@ contract DynamicAccountTest is BaseTest {
472484

473485
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
474486
accountSigner,
487+
0,
475488
approvedTargets,
476489
1 ether,
477490
0,
478491
type(uint128).max,
479492
0,
480493
type(uint128).max,
481-
uidCache,
482-
0
494+
uidCache
483495
);
484496

485497
vm.prank(accountAdmin);
@@ -510,14 +522,14 @@ contract DynamicAccountTest is BaseTest {
510522

511523
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
512524
accountSigner,
525+
0,
513526
approvedTargets,
514527
1 ether,
515528
0,
516529
type(uint128).max,
517530
0,
518531
type(uint128).max,
519-
uidCache,
520-
0
532+
uidCache
521533
);
522534

523535
vm.prank(accountAdmin);
@@ -568,14 +580,14 @@ contract DynamicAccountTest is BaseTest {
568580

569581
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
570582
accountSigner,
583+
0,
571584
approvedTargets,
572585
1 ether,
573586
0,
574587
type(uint128).max,
575588
0,
576589
type(uint128).max,
577-
uidCache,
578-
0
590+
uidCache
579591
);
580592

581593
vm.prank(accountAdmin);

0 commit comments

Comments
 (0)