Skip to content

Commit 498d7dd

Browse files
WhiteOakKongjoaquim-vergeskumaryash90
authored
add wildcard target to isValidSigner/isValidSignature (#539)
* add wildcard target * fix nits * lint/clean * fix test * tree for isValidSigner * re-add solhint disable * optimize to reduce contract size * fix expectrevert * test isValidSigner --------- Co-authored-by: Joaquim Verges <joaquim.verges@gmail.com> Co-authored-by: Yash <kumaryashcse@gmail.com>
1 parent 4161cab commit 498d7dd

File tree

8 files changed

+680
-47
lines changed

8 files changed

+680
-47
lines changed

‎contracts/extension/upgradeable/AccountPermissions.sol‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,8 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
4545
"SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
4646
);
4747

48-
modifier onlyAdmin() virtual {
48+
function _onlyAdmin() internal virtual {
4949
require(isAdmin(msg.sender), "!admin");
50-
_;
5150
}
5251

5352
/*///////////////////////////////////////////////////////////////
@@ -78,7 +77,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
7877
return;
7978
}
8079

81-
require(!isAdmin(targetSigner), "already admin");
80+
require(!isAdmin(targetSigner), "admin");
8281

8382
_accountPermissionsStorage().allSigners.add(targetSigner);
8483

@@ -89,13 +88,13 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
8988
);
9089

9190
address[] memory currentTargets = _accountPermissionsStorage().approvedTargets[targetSigner].values();
92-
uint256 currentLen = currentTargets.length;
91+
uint256 len = currentTargets.length;
9392

94-
for (uint256 i = 0; i < currentLen; i += 1) {
93+
for (uint256 i = 0; i < len; i += 1) {
9594
_accountPermissionsStorage().approvedTargets[targetSigner].remove(currentTargets[i]);
9695
}
9796

98-
uint256 len = _req.approvedTargets.length;
97+
len = _req.approvedTargets.length;
9998
for (uint256 i = 0; i < len; i += 1) {
10099
_accountPermissionsStorage().approvedTargets[targetSigner].add(_req.approvedTargets[i]);
101100
}

‎contracts/prebuilts/account/non-upgradeable/Account.sol‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,10 @@ contract Account is AccountCore, ContractMetadata, ERC1271, ERC721Holder, ERC115
7575
}
7676

7777
address caller = msg.sender;
78+
EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[signer];
79+
7880
require(
79-
_accountPermissionsStorage().approvedTargets[signer].contains(caller),
81+
approvedTargets.contains(caller) || (approvedTargets.length() == 1 && approvedTargets.at(0) == address(0)),
8082
"Account: caller not approved target."
8183
);
8284

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

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,21 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
7676
}
7777

7878
/// @notice Returns whether a signer is authorized to perform transactions using the wallet.
79+
/* solhint-disable*/
7980
function isValidSigner(address _signer, UserOperation calldata _userOp) public view virtual returns (bool) {
8081
// First, check if the signer is an admin.
8182
if (_accountPermissionsStorage().isAdmin[_signer]) {
8283
return true;
8384
}
8485

8586
SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[_signer];
87+
EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[_signer];
8688

8789
// If not an admin, check if the signer is active.
8890
if (
8991
permissions.startTimestamp > block.timestamp ||
9092
block.timestamp >= permissions.endTimestamp ||
91-
_accountPermissionsStorage().approvedTargets[_signer].length() == 0
93+
approvedTargets.length() == 0
9294
) {
9395
// Account: no active permissions.
9496
return false;
@@ -97,28 +99,44 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
9799
// Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`.
98100
bytes4 sig = getFunctionSignature(_userOp.callData);
99101

102+
// if address(0) is the only approved target, set isWildCard to true (wildcard approved).
103+
bool isWildCard = approvedTargets.length() == 1 && approvedTargets.at(0) == address(0);
104+
100105
if (sig == AccountExtension.execute.selector) {
101106
// Extract the `target` and `value` arguments from the calldata for `execute`.
102107
(address target, uint256 value) = decodeExecuteCalldata(_userOp.callData);
103108

109+
// if wildcard target is not approved, check that the target is in the approvedTargets set.
110+
if (!isWildCard) {
111+
// Check if the target is approved.
112+
if (!approvedTargets.contains(target)) {
113+
// Account: target not approved.
114+
return false;
115+
}
116+
}
117+
104118
// Check if the value is within the allowed range and if the target is approved.
105-
if (
106-
permissions.nativeTokenLimitPerTransaction < value ||
107-
!_accountPermissionsStorage().approvedTargets[_signer].contains(target)
108-
) {
119+
if (permissions.nativeTokenLimitPerTransaction < value) {
109120
// Account: value too high OR Account: target not approved.
110121
return false;
111122
}
112123
} else if (sig == AccountExtension.executeBatch.selector) {
113124
// Extract the `target` and `value` array arguments from the calldata for `executeBatch`.
114125
(address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData);
115126

127+
// if wildcard target is not approved, check that the targets are in the approvedTargets set.
128+
if (!isWildCard) {
129+
for (uint256 i = 0; i < targets.length; i++) {
130+
if (!approvedTargets.contains(targets[i])) {
131+
// If any target is not approved, break the loop.
132+
return false;
133+
}
134+
}
135+
}
136+
116137
// For each target+value pair, check if the value is within the allowed range and if the target is approved.
117138
for (uint256 i = 0; i < targets.length; i++) {
118-
if (
119-
permissions.nativeTokenLimitPerTransaction < values[i] ||
120-
!_accountPermissionsStorage().approvedTargets[_signer].contains(targets[i])
121-
) {
139+
if (permissions.nativeTokenLimitPerTransaction < values[i]) {
122140
// Account: value too high OR Account: target not approved.
123141
return false;
124142
}
@@ -131,6 +149,8 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
131149
return true;
132150
}
133151

152+
/* solhint-enable */
153+
134154
/*///////////////////////////////////////////////////////////////
135155
External functions
136156
//////////////////////////////////////////////////////////////*/
@@ -141,12 +161,14 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
141161
}
142162

143163
/// @notice Withdraw funds for this account from Entrypoint.
144-
function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyAdmin {
164+
function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public {
165+
_onlyAdmin();
145166
entryPoint().withdrawTo(withdrawAddress, amount);
146167
}
147168

148169
/// @notice Overrides the Entrypoint contract being used.
149-
function setEntrypointOverride(IEntryPoint _entrypointOverride) public virtual onlyAdmin {
170+
function setEntrypointOverride(IEntryPoint _entrypointOverride) public virtual {
171+
_onlyAdmin();
150172
AccountCoreStorage.data().entrypointOverride = address(_entrypointOverride);
151173
}
152174

@@ -195,8 +217,10 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
195217

196218
if (!isValidSigner(signer, userOp)) return SIG_VALIDATION_FAILED;
197219

198-
uint48 validAfter = uint48(_accountPermissionsStorage().signerPermissions[signer].startTimestamp);
199-
uint48 validUntil = uint48(_accountPermissionsStorage().signerPermissions[signer].endTimestamp);
220+
SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[signer];
221+
222+
uint48 validAfter = uint48(permissions.startTimestamp);
223+
uint48 validUntil = uint48(permissions.endTimestamp);
200224

201225
return _packValidationData(ValidationData(address(0), validAfter, validUntil));
202226
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,10 @@ contract AccountExtension is ContractMetadata, ERC1271, AccountPermissions, ERC7
7777
}
7878

7979
address caller = msg.sender;
80+
EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[signer];
81+
8082
require(
81-
_accountPermissionsStorage().approvedTargets[signer].contains(caller),
83+
approvedTargets.contains(caller) || (approvedTargets.length() == 1 && approvedTargets.at(0) == address(0)),
8284
"Account: caller not approved target."
8385
);
8486

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ contract SimpleAccountVulnPOCTest is BaseTest {
243243
Setup
244244
//////////////////////////////////////////////////////////////*/
245245
address[] memory approvedTargets = new address[](1);
246-
approvedTargets[0] = address(0); // allowing accountSigner permissions for some random contract, consider it as 0 address here
246+
approvedTargets[0] = address(0x123); // allowing accountSigner permissions for some random contract, consider it as 0 address here
247247

248248
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
249249
accountSigner,

0 commit comments

Comments
 (0)