Skip to content

Commit e7fd7f7

Browse files
nkrishangjoaquim-verges
nkrishang
and
joaquim-verges
authored
Smart Account contracts: audit fixes (#396)
* rename account factory callbacks * Create and use AccountPermissions * Fix [M-3] Native tokens can get locked in ManagedAccountFactory * cleanup * Fix [L-1, L-2] * Fix [Q-1] Potentially susceptible to signature malleability * Fix [Q-2, Q-3, Q-4, G-1] * update tests * Fix isValidSignature logic for non-admins * Fix Appendix A-2 * Fix Appendix A-3 * make modifiers virtual and return result from internal _call function * Add ContractMetadata to all account factories * Add some checks to factory callbacks * gas benchmarks * docs update * pkg release * pkg update * pkg update --------- Co-authored-by: Joaquim Verges <[email protected]>
1 parent 266fcde commit e7fd7f7

33 files changed

+3039
-706
lines changed

‎contracts/dynamic-contracts/extension/AccountPermissions.sol

+203
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
pragma solidity ^0.8.0;
3+
4+
/// @author thirdweb
5+
6+
import "../../extension/interface/IAccountPermissions.sol";
7+
import "../../openzeppelin-presets/utils/cryptography/EIP712.sol";
8+
import "../../openzeppelin-presets/utils/structs/EnumerableSet.sol";
9+
10+
library AccountPermissionsStorage {
11+
bytes32 public constant ACCOUNT_PERMISSIONS_STORAGE_POSITION = keccak256("account.permissions.storage");
12+
13+
struct Data {
14+
/// @dev Map from address => whether the address is an admin.
15+
mapping(address => bool) isAdmin;
16+
/// @dev Map from keccak256 hash of a role => active restrictions for that role.
17+
mapping(bytes32 => IAccountPermissions.RoleStatic) roleRestrictions;
18+
/// @dev Map from address => the role held by that address.
19+
mapping(address => bytes32) roleOfAccount;
20+
/// @dev Mapping from a signed request UID => whether the request is processed.
21+
mapping(bytes32 => bool) executed;
22+
/// @dev Map from keccak256 hash of a role to its approved targets.
23+
mapping(bytes32 => EnumerableSet.AddressSet) approvedTargets;
24+
/// @dev map from keccak256 hash of a role to its members' data. See {RoleMembers}.
25+
mapping(bytes32 => EnumerableSet.AddressSet) roleMembers;
26+
}
27+
28+
function accountPermissionsStorage() internal pure returns (Data storage accountPermissionsData) {
29+
bytes32 position = ACCOUNT_PERMISSIONS_STORAGE_POSITION;
30+
assembly {
31+
accountPermissionsData.slot := position
32+
}
33+
}
34+
}
35+
36+
abstract contract AccountPermissions is IAccountPermissions, EIP712 {
37+
using ECDSA for bytes32;
38+
using EnumerableSet for EnumerableSet.AddressSet;
39+
40+
bytes32 private constant TYPEHASH =
41+
keccak256(
42+
"RoleRequest(bytes32 role,address target,uint8 action,uint128 validityStartTimestamp,uint128 validityEndTimestamp,bytes32 uid)"
43+
);
44+
45+
modifier onlyAdmin() virtual {
46+
require(isAdmin(msg.sender), "AccountPermissions: caller is not an admin");
47+
_;
48+
}
49+
50+
/*///////////////////////////////////////////////////////////////
51+
External functions
52+
//////////////////////////////////////////////////////////////*/
53+
54+
/// @notice Adds / removes an account as an admin.
55+
function setAdmin(address _account, bool _isAdmin) external virtual onlyAdmin {
56+
_setAdmin(_account, _isAdmin);
57+
}
58+
59+
/// @notice Sets the restrictions for a given role.
60+
function setRoleRestrictions(RoleRestrictions calldata _restrictions) external virtual onlyAdmin {
61+
require(_restrictions.role != bytes32(0), "AccountPermissions: role cannot be empty");
62+
63+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
64+
data.roleRestrictions[_restrictions.role] = RoleStatic(
65+
_restrictions.role,
66+
_restrictions.maxValuePerTransaction,
67+
_restrictions.startTimestamp,
68+
_restrictions.endTimestamp
69+
);
70+
71+
uint256 len = _restrictions.approvedTargets.length;
72+
delete data.approvedTargets[_restrictions.role];
73+
for (uint256 i = 0; i < len; i++) {
74+
data.approvedTargets[_restrictions.role].add(_restrictions.approvedTargets[i]);
75+
}
76+
77+
emit RoleUpdated(_restrictions.role, _restrictions);
78+
}
79+
80+
/// @notice Grant / revoke a role from a given signer.
81+
function changeRole(RoleRequest calldata _req, bytes calldata _signature) external virtual {
82+
require(_req.role != bytes32(0), "AccountPermissions: role cannot be empty");
83+
require(
84+
_req.validityStartTimestamp < block.timestamp && block.timestamp < _req.validityEndTimestamp,
85+
"AccountPermissions: invalid validity period"
86+
);
87+
88+
(bool success, address signer) = verifyRoleRequest(_req, _signature);
89+
90+
require(success, "AccountPermissions: invalid signature");
91+
92+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
93+
data.executed[_req.uid] = true;
94+
95+
if (_req.action == RoleAction.GRANT) {
96+
data.roleOfAccount[_req.target] = _req.role;
97+
data.roleMembers[_req.role].add(_req.target);
98+
} else {
99+
delete data.roleOfAccount[_req.target];
100+
data.roleMembers[_req.role].remove(_req.target);
101+
}
102+
103+
emit RoleAssignment(_req.role, _req.target, signer, _req);
104+
}
105+
106+
/*///////////////////////////////////////////////////////////////
107+
View functions
108+
//////////////////////////////////////////////////////////////*/
109+
110+
/// @notice Returns whether the given account is an admin.
111+
function isAdmin(address _account) public view virtual returns (bool) {
112+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
113+
return data.isAdmin[_account];
114+
}
115+
116+
/// @notice Returns the role held by a given account along with its restrictions.
117+
function getRoleRestrictionsForAccount(address _account) external view virtual returns (RoleRestrictions memory) {
118+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
119+
bytes32 role = data.roleOfAccount[_account];
120+
RoleStatic memory roleRestrictions = data.roleRestrictions[role];
121+
122+
return
123+
RoleRestrictions(
124+
role,
125+
data.approvedTargets[role].values(),
126+
roleRestrictions.maxValuePerTransaction,
127+
roleRestrictions.startTimestamp,
128+
roleRestrictions.endTimestamp
129+
);
130+
}
131+
132+
/// @notice Returns the role restrictions for a given role.
133+
function getRoleRestrictions(bytes32 _role) external view virtual returns (RoleRestrictions memory) {
134+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
135+
RoleStatic memory roleRestrictions = data.roleRestrictions[_role];
136+
137+
return
138+
RoleRestrictions(
139+
_role,
140+
data.approvedTargets[_role].values(),
141+
roleRestrictions.maxValuePerTransaction,
142+
roleRestrictions.startTimestamp,
143+
roleRestrictions.endTimestamp
144+
);
145+
}
146+
147+
/// @notice Returns all accounts that have a role.
148+
function getAllRoleMembers(bytes32 _role) external view virtual returns (address[] memory) {
149+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
150+
return data.roleMembers[_role].values();
151+
}
152+
153+
/// @dev Verifies that a request is signed by an authorized account.
154+
function verifyRoleRequest(RoleRequest calldata req, bytes calldata signature)
155+
public
156+
view
157+
virtual
158+
returns (bool success, address signer)
159+
{
160+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
161+
signer = _recoverAddress(req, signature);
162+
success = !data.executed[req.uid] && isAdmin(signer);
163+
}
164+
165+
/*///////////////////////////////////////////////////////////////
166+
Internal functions
167+
//////////////////////////////////////////////////////////////*/
168+
169+
/// @notice Runs after every `changeRole` run.
170+
function _afterChangeRole(RoleRequest calldata _req) internal virtual;
171+
172+
/// @notice Makes the given account an admin.
173+
function _setAdmin(address _account, bool _isAdmin) internal virtual {
174+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
175+
data.isAdmin[_account] = _isAdmin;
176+
177+
emit AdminUpdated(_account, _isAdmin);
178+
}
179+
180+
/// @dev Returns the address of the signer of the request.
181+
function _recoverAddress(RoleRequest calldata _req, bytes calldata _signature)
182+
internal
183+
view
184+
virtual
185+
returns (address)
186+
{
187+
return _hashTypedDataV4(keccak256(_encodeRequest(_req))).recover(_signature);
188+
}
189+
190+
/// @dev Encodes a request for recovery of the signer in `recoverAddress`.
191+
function _encodeRequest(RoleRequest calldata _req) internal pure virtual returns (bytes memory) {
192+
return
193+
abi.encode(
194+
TYPEHASH,
195+
_req.role,
196+
_req.target,
197+
_req.action,
198+
_req.validityStartTimestamp,
199+
_req.validityEndTimestamp,
200+
_req.uid
201+
);
202+
}
203+
}

‎contracts/extension/interface/IAccountPermissions.sol

+115
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 {
7+
/*///////////////////////////////////////////////////////////////
8+
Types
9+
//////////////////////////////////////////////////////////////*/
10+
11+
/// @notice Roles can be granted or revoked by an authorized party.
12+
enum RoleAction {
13+
GRANT,
14+
REVOKE
15+
}
16+
17+
/**
18+
* @notice The payload that must be signed by an authorized wallet to grant / revoke a role.
19+
*
20+
* @param role The role to grant / revoke.
21+
* @param target The address to grant / revoke the role from.
22+
* @param action Whether to grant or revoke the role.
23+
* @param validityStartTimestamp The UNIX timestamp at and after which a signature is valid.
24+
* @param validityEndTimestamp The UNIX timestamp at and after which a signature is invalid/expired.
25+
* @param uid A unique non-repeatable ID for the payload.
26+
*/
27+
struct RoleRequest {
28+
bytes32 role;
29+
address target;
30+
RoleAction action;
31+
uint128 validityStartTimestamp;
32+
uint128 validityEndTimestamp;
33+
bytes32 uid;
34+
}
35+
36+
/**
37+
* @notice Restrictions that can be applied to a given role.
38+
*
39+
* @param role The unique role identifier.
40+
* @param approvedTargets The list of approved targets that a role holder can call using the smart wallet.
41+
* @param maxValuePerTransaction The maximum value that can be transferred by a role holder in a single transaction.
42+
* @param startTimestamp The UNIX timestamp at and after which a role holder can call the approved targets.
43+
* @param endTimestamp The UNIX timestamp at and after which a role holder can no longer call the approved targets.
44+
*/
45+
struct RoleRestrictions {
46+
bytes32 role;
47+
address[] approvedTargets;
48+
uint256 maxValuePerTransaction;
49+
uint128 startTimestamp;
50+
uint128 endTimestamp;
51+
}
52+
53+
/**
54+
* @notice Internal struct for storing roles without approved targets
55+
*
56+
* @param role The unique role identifier.
57+
* @param maxValuePerTransaction The maximum value that can be transferred by a role holder in a single transaction.
58+
* @param startTimestamp The UNIX timestamp at and after which a role holder can call the approved targets.
59+
* @param endTimestamp The UNIX timestamp at and after which a role holder can no longer call the approved targets.
60+
*/
61+
struct RoleStatic {
62+
bytes32 role;
63+
uint256 maxValuePerTransaction;
64+
uint128 startTimestamp;
65+
uint128 endTimestamp;
66+
}
67+
68+
/*///////////////////////////////////////////////////////////////
69+
Events
70+
//////////////////////////////////////////////////////////////*/
71+
72+
/// @notice Emitted when the restrictions for a given role are updated.
73+
event RoleUpdated(bytes32 indexed role, RoleRestrictions restrictions);
74+
75+
/// @notice Emitted when a role is granted / revoked by an authorized party.
76+
event RoleAssignment(bytes32 indexed role, address indexed account, address indexed signer, RoleRequest request);
77+
78+
/// @notice Emitted when an admin is set or removed.
79+
event AdminUpdated(address indexed account, bool isAdmin);
80+
81+
/*///////////////////////////////////////////////////////////////
82+
View functions
83+
//////////////////////////////////////////////////////////////*/
84+
85+
/// @notice Returns whether the given account is an admin.
86+
function isAdmin(address account) external view returns (bool);
87+
88+
/// @notice Returns the role held by a given account along with its restrictions.
89+
function getRoleRestrictionsForAccount(address account) external view returns (RoleRestrictions memory role);
90+
91+
/// @notice Returns the role restrictions for a given role.
92+
function getRoleRestrictions(bytes32 role) external view returns (RoleRestrictions memory restrictions);
93+
94+
/// @notice Returns all accounts that have a role.
95+
function getAllRoleMembers(bytes32 role) external view returns (address[] memory members);
96+
97+
/// @dev Verifies that a request is signed by an authorized account.
98+
function verifyRoleRequest(RoleRequest calldata req, bytes calldata signature)
99+
external
100+
view
101+
returns (bool success, address signer);
102+
103+
/*///////////////////////////////////////////////////////////////
104+
External functions
105+
//////////////////////////////////////////////////////////////*/
106+
107+
/// @notice Adds / removes an account as an admin.
108+
function setAdmin(address account, bool isAdmin) external;
109+
110+
/// @notice Sets the restrictions for a given role.
111+
function setRoleRestrictions(RoleRestrictions calldata role) external;
112+
113+
/// @notice Grant / revoke a role from a given signer.
114+
function changeRole(RoleRequest calldata req, bytes calldata signature) external;
115+
}

‎contracts/openzeppelin-presets/proxy/Clones.sol

+20-18
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// SPDX-License-Identifier: MIT
2-
// OpenZeppelin Contracts (last updated v4.7.0) (proxy/Clones.sol)
2+
// OpenZeppelin Contracts (last updated v4.8.0) (proxy/Clones.sol)
33

44
pragma solidity ^0.8.0;
55

@@ -25,11 +25,12 @@ library Clones {
2525
function clone(address implementation) internal returns (address instance) {
2626
/// @solidity memory-safe-assembly
2727
assembly {
28-
let ptr := mload(0x40)
29-
mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
30-
mstore(add(ptr, 0x14), shl(0x60, implementation))
31-
mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
32-
instance := create(0, ptr, 0x37)
28+
// Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
29+
// of the `implementation` address with the bytecode before the address.
30+
mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
31+
// Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
32+
mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
33+
instance := create(0, 0x09, 0x37)
3334
}
3435
require(instance != address(0), "ERC1167: create failed");
3536
}
@@ -44,11 +45,12 @@ library Clones {
4445
function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
4546
/// @solidity memory-safe-assembly
4647
assembly {
47-
let ptr := mload(0x40)
48-
mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
49-
mstore(add(ptr, 0x14), shl(0x60, implementation))
50-
mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
51-
instance := create2(0, ptr, 0x37, salt)
48+
// Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
49+
// of the `implementation` address with the bytecode before the address.
50+
mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
51+
// Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
52+
mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
53+
instance := create2(0, 0x09, 0x37, salt)
5254
}
5355
require(instance != address(0), "ERC1167: create2 failed");
5456
}
@@ -64,13 +66,13 @@ library Clones {
6466
/// @solidity memory-safe-assembly
6567
assembly {
6668
let ptr := mload(0x40)
67-
mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
68-
mstore(add(ptr, 0x14), shl(0x60, implementation))
69-
mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf3ff00000000000000000000000000000000)
70-
mstore(add(ptr, 0x38), shl(0x60, deployer))
71-
mstore(add(ptr, 0x4c), salt)
72-
mstore(add(ptr, 0x6c), keccak256(ptr, 0x37))
73-
predicted := keccak256(add(ptr, 0x37), 0x55)
69+
mstore(add(ptr, 0x38), deployer)
70+
mstore(add(ptr, 0x24), 0x5af43d82803e903d91602b57fd5bf3ff)
71+
mstore(add(ptr, 0x14), implementation)
72+
mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73)
73+
mstore(add(ptr, 0x58), salt)
74+
mstore(add(ptr, 0x78), keccak256(add(ptr, 0x0c), 0x37))
75+
predicted := keccak256(add(ptr, 0x43), 0x55)
7476
}
7577
}
7678

0 commit comments

Comments
 (0)