Skip to content

Commit 6edce25

Browse files
WhiteOakKongkumaryash90joaquim-verges
authored
AccountPermissions: add setAdminWithSigner (#533)
* setAdminWithSigner * fix typo * restructure setAdminWithSigner * reduce size * add AccountPermissions to legacy-contracts * update struct SignerPermissionRequest * cleanup, release script change * test: setPermissionsForSigner * release * dev release --------- Co-authored-by: Yash <kumaryashcse@gmail.com> Co-authored-by: Joaquim Verges <joaquim.verges@gmail.com>
1 parent 516e229 commit 6edce25

File tree

11 files changed

+874
-53
lines changed

11 files changed

+874
-53
lines changed

‎contracts/extension/interface/IAccountPermissions.sol‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ interface IAccountPermissions {
1919
* @param reqValidityStartTimestamp The UNIX timestamp at and after which a signature is valid.
2020
* @param reqValidityEndTimestamp The UNIX timestamp at and after which a signature is invalid/expired.
2121
* @param uid A unique non-repeatable ID for the payload.
22+
* @param isAdmin Whether the signer should be an admin.
2223
*/
2324
struct SignerPermissionRequest {
2425
address signer;
@@ -29,6 +30,7 @@ interface IAccountPermissions {
2930
uint128 reqValidityStartTimestamp;
3031
uint128 reqValidityEndTimestamp;
3132
bytes32 uid;
33+
uint8 isAdmin;
3234
}
3335

3436
/**
@@ -107,9 +109,6 @@ interface IAccountPermissions {
107109
External functions
108110
//////////////////////////////////////////////////////////////*/
109111

110-
/// @notice Adds / removes an account as an admin.
111-
function setAdmin(address account, bool isAdmin) external;
112-
113112
/// @notice Sets the permissions for a given signer.
114113
function setPermissionsForSigner(SignerPermissionRequest calldata req, bytes calldata signature) external;
115114
}

‎contracts/extension/upgradeable/AccountPermissions.sol‎

Lines changed: 34 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -46,35 +46,42 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
4646
);
4747

4848
modifier onlyAdmin() virtual {
49-
require(isAdmin(msg.sender), "caller is not an admin");
49+
require(isAdmin(msg.sender), "!admin");
5050
_;
5151
}
5252

5353
/*///////////////////////////////////////////////////////////////
5454
External functions
5555
//////////////////////////////////////////////////////////////*/
5656

57-
/// @notice Adds / removes an account as an admin.
58-
function setAdmin(address _account, bool _isAdmin) external virtual onlyAdmin {
59-
_setAdmin(_account, _isAdmin);
60-
}
61-
6257
/// @notice Sets the permissions for a given signer.
6358
function setPermissionsForSigner(SignerPermissionRequest calldata _req, bytes calldata _signature) external {
6459
address targetSigner = _req.signer;
65-
require(!isAdmin(targetSigner), "signer is already an admin");
6660

6761
require(
6862
_req.reqValidityStartTimestamp <= block.timestamp && block.timestamp < _req.reqValidityEndTimestamp,
69-
"invalid request validity period"
63+
"!period"
7064
);
7165

7266
(bool success, address signer) = verifySignerPermissionRequest(_req, _signature);
73-
require(success, "invalid signature");
67+
require(success, "!sig");
7468

75-
_accountPermissionsStorage().allSigners.add(targetSigner);
7669
_accountPermissionsStorage().executed[_req.uid] = true;
7770

71+
//isAdmin > 0, set admin or remove admin
72+
if (_req.isAdmin > 0) {
73+
//isAdmin = 1, set admin
74+
//isAdmin > 1, remove admin
75+
bool _isAdmin = _req.isAdmin == 1;
76+
77+
_setAdmin(targetSigner, _isAdmin);
78+
return;
79+
}
80+
81+
require(!isAdmin(targetSigner), "already admin");
82+
83+
_accountPermissionsStorage().allSigners.add(targetSigner);
84+
7885
_accountPermissionsStorage().signerPermissions[targetSigner] = SignerPermissionsStatic(
7986
_req.nativeTokenLimitPerTransaction,
8087
_req.permissionStartTimestamp,
@@ -138,7 +145,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
138145
virtual
139146
returns (bool success, address signer)
140147
{
141-
signer = _recoverAddress(req, signature);
148+
signer = _recoverAddress(_encodeRequest(req), signature);
142149
success = !_accountPermissionsStorage().executed[req.uid] && isAdmin(signer);
143150
}
144151

@@ -168,34 +175,30 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
168175

169176
uint256 len = allSigners.length;
170177
uint256 numOfActiveSigners = 0;
171-
bool[] memory isSignerActive = new bool[](len);
172178

173179
for (uint256 i = 0; i < len; i += 1) {
174-
address signer = allSigners[i];
175-
176-
bool isActive = isActiveSigner(signer);
177-
isSignerActive[i] = isActive;
178-
if (isActive) {
180+
if (isActiveSigner(allSigners[i])) {
179181
numOfActiveSigners++;
182+
} else {
183+
allSigners[i] = address(0);
180184
}
181185
}
182186

183187
signers = new SignerPermissions[](numOfActiveSigners);
184188
uint256 index = 0;
185189
for (uint256 i = 0; i < len; i += 1) {
186-
if (!isSignerActive[i]) {
187-
continue;
190+
if (allSigners[i] != address(0)) {
191+
address signer = allSigners[i];
192+
SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[signer];
193+
194+
signers[index++] = SignerPermissions(
195+
signer,
196+
_accountPermissionsStorage().approvedTargets[signer].values(),
197+
permissions.nativeTokenLimitPerTransaction,
198+
permissions.startTimestamp,
199+
permissions.endTimestamp
200+
);
188201
}
189-
address signer = allSigners[i];
190-
SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[signer];
191-
192-
signers[index++] = SignerPermissions(
193-
signer,
194-
_accountPermissionsStorage().approvedTargets[signer].values(),
195-
permissions.nativeTokenLimitPerTransaction,
196-
permissions.startTimestamp,
197-
permissions.endTimestamp
198-
);
199202
}
200203
}
201204

@@ -225,13 +228,8 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
225228
}
226229

227230
/// @dev Returns the address of the signer of the request.
228-
function _recoverAddress(SignerPermissionRequest calldata _req, bytes calldata _signature)
229-
internal
230-
view
231-
virtual
232-
returns (address)
233-
{
234-
return _hashTypedDataV4(keccak256(_encodeRequest(_req))).recover(_signature);
231+
function _recoverAddress(bytes memory _encoded, bytes calldata _signature) internal view virtual returns (address) {
232+
return _hashTypedDataV4(keccak256(_encoded)).recover(_signature);
235233
}
236234

237235
/// @dev Encodes a request for recovery of the signer in `recoverAddress`.

‎contracts/legacy-contracts/smart-wallet/interface/IAccountPermissions_V1.sol‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
pragma solidity ^0.8.0;
3+
4+
/// @author thirdweb
5+
6+
interface IAccountPermissions_V1 {
7+
/*///////////////////////////////////////////////////////////////
8+
Types
9+
//////////////////////////////////////////////////////////////*/
10+
11+
/**
12+
* @notice The payload that must be signed by an authorized wallet to set permissions for a signer to use the smart wallet.
13+
*
14+
* @param signer The addres of the signer to give permissions.
15+
* @param approvedTargets The list of approved targets that a role holder can call using the smart wallet.
16+
* @param nativeTokenLimitPerTransaction The maximum value that can be transferred by a role holder in a single transaction.
17+
* @param permissionStartTimestamp The UNIX timestamp at and after which a signer has permission to use the smart wallet.
18+
* @param permissionEndTimestamp The UNIX timestamp at and after which a signer no longer has permission to use the smart wallet.
19+
* @param reqValidityStartTimestamp The UNIX timestamp at and after which a signature is valid.
20+
* @param reqValidityEndTimestamp The UNIX timestamp at and after which a signature is invalid/expired.
21+
* @param uid A unique non-repeatable ID for the payload.
22+
*/
23+
struct SignerPermissionRequest {
24+
address signer;
25+
address[] approvedTargets;
26+
uint256 nativeTokenLimitPerTransaction;
27+
uint128 permissionStartTimestamp;
28+
uint128 permissionEndTimestamp;
29+
uint128 reqValidityStartTimestamp;
30+
uint128 reqValidityEndTimestamp;
31+
bytes32 uid;
32+
}
33+
34+
/**
35+
* @notice The permissions that a signer has to use the smart wallet.
36+
*
37+
* @param signer The address of the signer.
38+
* @param approvedTargets The list of approved targets that a role holder can call using the smart wallet.
39+
* @param nativeTokenLimitPerTransaction The maximum value that can be transferred by a role holder in a single transaction.
40+
* @param startTimestamp The UNIX timestamp at and after which a signer has permission to use the smart wallet.
41+
* @param endTimestamp The UNIX timestamp at and after which a signer no longer has permission to use the smart wallet.
42+
*/
43+
struct SignerPermissions {
44+
address signer;
45+
address[] approvedTargets;
46+
uint256 nativeTokenLimitPerTransaction;
47+
uint128 startTimestamp;
48+
uint128 endTimestamp;
49+
}
50+
51+
/**
52+
* @notice Internal struct for storing permissions for a signer (without approved targets).
53+
*
54+
* @param nativeTokenLimitPerTransaction The maximum value that can be transferred by a role holder in a single transaction.
55+
* @param startTimestamp The UNIX timestamp at and after which a signer has permission to use the smart wallet.
56+
* @param endTimestamp The UNIX timestamp at and after which a signer no longer has permission to use the smart wallet.
57+
*/
58+
struct SignerPermissionsStatic {
59+
uint256 nativeTokenLimitPerTransaction;
60+
uint128 startTimestamp;
61+
uint128 endTimestamp;
62+
}
63+
64+
/*///////////////////////////////////////////////////////////////
65+
Events
66+
//////////////////////////////////////////////////////////////*/
67+
68+
/// @notice Emitted when permissions for a signer are updated.
69+
event SignerPermissionsUpdated(
70+
address indexed authorizingSigner,
71+
address indexed targetSigner,
72+
SignerPermissionRequest permissions
73+
);
74+
75+
/// @notice Emitted when an admin is set or removed.
76+
event AdminUpdated(address indexed signer, bool isAdmin);
77+
78+
/*///////////////////////////////////////////////////////////////
79+
View functions
80+
//////////////////////////////////////////////////////////////*/
81+
82+
/// @notice Returns whether the given account is an admin.
83+
function isAdmin(address signer) external view returns (bool);
84+
85+
/// @notice Returns whether the given account is an active signer on the account.
86+
function isActiveSigner(address signer) external view returns (bool);
87+
88+
/// @notice Returns the restrictions under which a signer can use the smart wallet.
89+
function getPermissionsForSigner(address signer) external view returns (SignerPermissions memory permissions);
90+
91+
/// @notice Returns all active and inactive signers of the account.
92+
function getAllSigners() external view returns (SignerPermissions[] memory signers);
93+
94+
/// @notice Returns all signers with active permissions to use the account.
95+
function getAllActiveSigners() external view returns (SignerPermissions[] memory signers);
96+
97+
/// @notice Returns all admins of the account.
98+
function getAllAdmins() external view returns (address[] memory admins);
99+
100+
/// @dev Verifies that a request is signed by an authorized account.
101+
function verifySignerPermissionRequest(SignerPermissionRequest calldata req, bytes calldata signature)
102+
external
103+
view
104+
returns (bool success, address signer);
105+
106+
/*///////////////////////////////////////////////////////////////
107+
External functions
108+
//////////////////////////////////////////////////////////////*/
109+
110+
/// @notice Adds / removes an account as an admin.
111+
function setAdmin(address account, bool isAdmin) external;
112+
113+
/// @notice Sets the permissions for a given signer.
114+
function setPermissionsForSigner(SignerPermissionRequest calldata req, bytes calldata signature) external;
115+
}

‎contracts/package.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,8 @@
2222
"@openzeppelin/contracts-upgradeable": "4.7.3",
2323
"erc721a-upgradeable": "^3.3.0",
2424
"@thirdweb-dev/dynamic-contracts": "^1.1.2"
25+
},
26+
"engines": {
27+
"node": ">=20.0.0"
2528
}
2629
}

‎contracts/prebuilts/account/utils/AccountCore.sol‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,12 +155,12 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
155155
//////////////////////////////////////////////////////////////*/
156156

157157
function getFunctionSignature(bytes calldata data) internal pure returns (bytes4 functionSelector) {
158-
require(data.length >= 4, "Data too short");
158+
require(data.length >= 4, "!Data");
159159
return bytes4(data[:4]);
160160
}
161161

162162
function decodeExecuteCalldata(bytes calldata data) internal pure returns (address _target, uint256 _value) {
163-
require(data.length >= 4 + 32 + 32, "Data too short");
163+
require(data.length >= 4 + 32 + 32, "!Data");
164164

165165
// Decode the address, which is bytes 4 to 35
166166
_target = abi.decode(data[4:36], (address));
@@ -178,7 +178,7 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
178178
bytes[] memory _callData
179179
)
180180
{
181-
require(data.length >= 4 + 32 + 32 + 32, "Data too short");
181+
require(data.length >= 4 + 32 + 32 + 32, "!Data");
182182

183183
(_targets, _values, _callData) = abi.decode(data[4:], (address[], uint256[], bytes[]));
184184
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,8 @@ contract SimpleAccountTest is BaseTest {
366366
type(uint128).max,
367367
0,
368368
type(uint128).max,
369-
uidCache
369+
uidCache,
370+
0
370371
);
371372

372373
vm.prank(accountAdmin);
@@ -403,7 +404,8 @@ contract SimpleAccountTest is BaseTest {
403404
type(uint128).max,
404405
0,
405406
type(uint128).max,
406-
uidCache
407+
uidCache,
408+
0
407409
);
408410

409411
vm.prank(accountAdmin);
@@ -458,7 +460,8 @@ contract SimpleAccountTest is BaseTest {
458460
type(uint128).max,
459461
0,
460462
type(uint128).max,
461-
uidCache
463+
uidCache,
464+
0
462465
);
463466

464467
vm.prank(accountAdmin);
@@ -606,7 +609,8 @@ contract SimpleAccountTest is BaseTest {
606609
type(uint128).max,
607610
0,
608611
type(uint128).max,
609-
uidCache
612+
uidCache,
613+
0
610614
);
611615

612616
vm.prank(accountAdmin);

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ contract SimpleAccountVulnPOCTest is BaseTest {
241241
type(uint128).max,
242242
0,
243243
type(uint128).max,
244-
uidCache
244+
uidCache,
245+
0
245246
);
246247

247248
vm.prank(accountAdmin);

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,8 @@ contract DynamicAccountTest is BaseTest {
478478
type(uint128).max,
479479
0,
480480
type(uint128).max,
481-
uidCache
481+
uidCache,
482+
0
482483
);
483484

484485
vm.prank(accountAdmin);
@@ -515,7 +516,8 @@ contract DynamicAccountTest is BaseTest {
515516
type(uint128).max,
516517
0,
517518
type(uint128).max,
518-
uidCache
519+
uidCache,
520+
0
519521
);
520522

521523
vm.prank(accountAdmin);
@@ -572,7 +574,8 @@ contract DynamicAccountTest is BaseTest {
572574
type(uint128).max,
573575
0,
574576
type(uint128).max,
575-
uidCache
577+
uidCache,
578+
0
576579
);
577580

578581
vm.prank(accountAdmin);

0 commit comments

Comments
 (0)