Skip to content

Commit c7c369d

Browse files
0xFirekeepernkrishang
0xFirekeeper
and
nkrishang
authored
[Account] Return false instead of reverting to allow for dummy simulations (#444)
* [Account] Return false instead of reverting to allow for dummy simulations. * Don't revert in isValidSigner --------- Co-authored-by: Krishang <krishang@thirdweb.com>
1 parent dc0a24b commit c7c369d

File tree

2 files changed

+87
-66
lines changed

2 files changed

+87
-66
lines changed

‎contracts/smart-wallet/non-upgradeable/Account.sol‎

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -107,42 +107,52 @@ contract Account is
107107
// First, check if the signer is an admin.
108108
if (data.isAdmin[_signer]) {
109109
return true;
110-
} else {
111-
SignerPermissionsStatic memory permissions = data.signerPermissions[_signer];
112-
113-
// If not an admin, check if the signer is active.
114-
require(
115-
permissions.startTimestamp <= block.timestamp &&
116-
block.timestamp < permissions.endTimestamp &&
117-
data.approvedTargets[_signer].length() > 0,
118-
"Account: no active permissions."
119-
);
120-
121-
// Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`.
122-
bytes4 sig = getFunctionSignature(_userOp.callData);
123-
124-
if (sig == this.execute.selector) {
125-
// Extract the `target` and `value` arguments from the calldata for `execute`.
126-
(address target, uint256 value) = decodeExecuteCalldata(_userOp.callData);
127-
128-
// Check if the value is within the allowed range and if the target is approved.
129-
require(permissions.nativeTokenLimitPerTransaction >= value, "Account: value too high.");
130-
require(data.approvedTargets[_signer].contains(target), "Account: target not approved.");
131-
} else if (sig == this.executeBatch.selector) {
132-
// Extract the `target` and `value` array arguments from the calldata for `executeBatch`.
133-
(address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData);
134-
135-
// For each target+value pair, check if the value is within the allowed range and if the target is approved.
136-
for (uint256 i = 0; i < targets.length; i++) {
137-
require(permissions.nativeTokenLimitPerTransaction >= values[i], "Account: value too high.");
138-
require(data.approvedTargets[_signer].contains(targets[i]), "Account: target not approved.");
110+
}
111+
112+
SignerPermissionsStatic memory permissions = data.signerPermissions[_signer];
113+
114+
// If not an admin, check if the signer is active.
115+
if (
116+
permissions.startTimestamp > block.timestamp ||
117+
block.timestamp >= permissions.endTimestamp ||
118+
data.approvedTargets[_signer].length() == 0
119+
) {
120+
// Account: no active permissions.
121+
return false;
122+
}
123+
124+
// Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`.
125+
bytes4 sig = getFunctionSignature(_userOp.callData);
126+
127+
if (sig == this.execute.selector) {
128+
// Extract the `target` and `value` arguments from the calldata for `execute`.
129+
(address target, uint256 value) = decodeExecuteCalldata(_userOp.callData);
130+
131+
// Check if the value is within the allowed range and if the target is approved.
132+
if (permissions.nativeTokenLimitPerTransaction < value || !data.approvedTargets[_signer].contains(target)) {
133+
// Account: value too high OR Account: target not approved.
134+
return false;
135+
}
136+
} else if (sig == this.executeBatch.selector) {
137+
// Extract the `target` and `value` array arguments from the calldata for `executeBatch`.
138+
(address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData);
139+
140+
// For each target+value pair, check if the value is within the allowed range and if the target is approved.
141+
for (uint256 i = 0; i < targets.length; i++) {
142+
if (
143+
permissions.nativeTokenLimitPerTransaction < values[i] ||
144+
!data.approvedTargets[_signer].contains(targets[i])
145+
) {
146+
// Account: value too high OR Account: target not approved.
147+
return false;
139148
}
140-
} else {
141-
revert("Account: calling invalid fn.");
142149
}
143-
144-
return true;
150+
} else {
151+
// Account: calling invalid fn.
152+
return false;
145153
}
154+
155+
return true;
146156
}
147157

148158
/// @notice See EIP-1271

‎contracts/smart-wallet/utils/AccountCore.sol‎

Lines changed: 44 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -78,48 +78,59 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, ERC
7878

7979
/// @notice Returns whether a signer is authorized to perform transactions using the wallet.
8080
function isValidSigner(address _signer, UserOperation calldata _userOp) public view virtual returns (bool) {
81+
// We use the underlying storage instead of high level view functions to save gas.
8182
// We use the underlying storage instead of high level view functions to save gas.
8283
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
8384

8485
// First, check if the signer is an admin.
8586
if (data.isAdmin[_signer]) {
8687
return true;
87-
} else {
88-
SignerPermissionsStatic memory permissions = data.signerPermissions[_signer];
89-
90-
// If not an admin, check if the signer is active.
91-
require(
92-
permissions.startTimestamp <= block.timestamp &&
93-
block.timestamp < permissions.endTimestamp &&
94-
data.approvedTargets[_signer].length() > 0,
95-
"Account: no active permissions."
96-
);
97-
98-
// Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`.
99-
bytes4 sig = getFunctionSignature(_userOp.callData);
100-
101-
if (sig == Account.execute.selector) {
102-
// Extract the `target` and `value` arguments from the calldata for `execute`.
103-
(address target, uint256 value) = decodeExecuteCalldata(_userOp.callData);
104-
105-
// Check if the value is within the allowed range and if the target is approved.
106-
require(permissions.nativeTokenLimitPerTransaction >= value, "Account: value too high.");
107-
require(data.approvedTargets[_signer].contains(target), "Account: target not approved.");
108-
} else if (sig == Account.executeBatch.selector) {
109-
// Extract the `target` and `value` array arguments from the calldata for `executeBatch`.
110-
(address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData);
111-
112-
// For each target+value pair, check if the value is within the allowed range and if the target is approved.
113-
for (uint256 i = 0; i < targets.length; i++) {
114-
require(permissions.nativeTokenLimitPerTransaction >= values[i], "Account: value too high.");
115-
require(data.approvedTargets[_signer].contains(targets[i]), "Account: target not approved.");
88+
}
89+
90+
SignerPermissionsStatic memory permissions = data.signerPermissions[_signer];
91+
92+
// If not an admin, check if the signer is active.
93+
if (
94+
permissions.startTimestamp > block.timestamp ||
95+
block.timestamp >= permissions.endTimestamp ||
96+
data.approvedTargets[_signer].length() == 0
97+
) {
98+
// Account: no active permissions.
99+
return false;
100+
}
101+
102+
// Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`.
103+
bytes4 sig = getFunctionSignature(_userOp.callData);
104+
105+
if (sig == Account.execute.selector) {
106+
// Extract the `target` and `value` arguments from the calldata for `execute`.
107+
(address target, uint256 value) = decodeExecuteCalldata(_userOp.callData);
108+
109+
// Check if the value is within the allowed range and if the target is approved.
110+
if (permissions.nativeTokenLimitPerTransaction < value || !data.approvedTargets[_signer].contains(target)) {
111+
// Account: value too high OR Account: target not approved.
112+
return false;
113+
}
114+
} else if (sig == Account.executeBatch.selector) {
115+
// Extract the `target` and `value` array arguments from the calldata for `executeBatch`.
116+
(address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData);
117+
118+
// For each target+value pair, check if the value is within the allowed range and if the target is approved.
119+
for (uint256 i = 0; i < targets.length; i++) {
120+
if (
121+
permissions.nativeTokenLimitPerTransaction < values[i] ||
122+
!data.approvedTargets[_signer].contains(targets[i])
123+
) {
124+
// Account: value too high OR Account: target not approved.
125+
return false;
116126
}
117-
} else {
118-
revert("Account: calling invalid fn.");
119127
}
120-
121-
return true;
128+
} else {
129+
// Account: calling invalid fn.
130+
return false;
122131
}
132+
133+
return true;
123134
}
124135

125136
/// @notice See EIP-1271

0 commit comments

Comments
 (0)