From 498d7ddaaf799006bdad4f2e963551ee37f46f3e Mon Sep 17 00:00:00 2001 From: WhiteOakKong <92236155+WhiteOakKong@users.noreply.github.com> Date: Fri, 13 Oct 2023 13:03:52 -0500 Subject: [PATCH] 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 Co-authored-by: Yash --- .../upgradeable/AccountPermissions.sol | 11 +- .../account/non-upgradeable/Account.sol | 4 +- .../prebuilts/account/utils/AccountCore.sol | 50 +- .../account/utils/AccountExtension.sol | 4 +- src/test/smart-wallet/AccountVulnPOC.t.sol | 2 +- .../account-core/isValidSigner.t.sol | 562 ++++++++++++++++++ .../account-core/isValidSigner.tree | 46 ++ .../setPermissionsForSigner.t.sol | 48 +- 8 files changed, 680 insertions(+), 47 deletions(-) create mode 100644 src/test/smart-wallet/account-core/isValidSigner.t.sol create mode 100644 src/test/smart-wallet/account-core/isValidSigner.tree diff --git a/contracts/extension/upgradeable/AccountPermissions.sol b/contracts/extension/upgradeable/AccountPermissions.sol index 0e9936359..bc9d9918a 100644 --- a/contracts/extension/upgradeable/AccountPermissions.sol +++ b/contracts/extension/upgradeable/AccountPermissions.sol @@ -45,9 +45,8 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 { "SignerPermissionRequest(address signer,uint8 isAdmin,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)" ); - modifier onlyAdmin() virtual { + function _onlyAdmin() internal virtual { require(isAdmin(msg.sender), "!admin"); - _; } /*/////////////////////////////////////////////////////////////// @@ -78,7 +77,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 { return; } - require(!isAdmin(targetSigner), "already admin"); + require(!isAdmin(targetSigner), "admin"); _accountPermissionsStorage().allSigners.add(targetSigner); @@ -89,13 +88,13 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 { ); address[] memory currentTargets = _accountPermissionsStorage().approvedTargets[targetSigner].values(); - uint256 currentLen = currentTargets.length; + uint256 len = currentTargets.length; - for (uint256 i = 0; i < currentLen; i += 1) { + for (uint256 i = 0; i < len; i += 1) { _accountPermissionsStorage().approvedTargets[targetSigner].remove(currentTargets[i]); } - uint256 len = _req.approvedTargets.length; + len = _req.approvedTargets.length; for (uint256 i = 0; i < len; i += 1) { _accountPermissionsStorage().approvedTargets[targetSigner].add(_req.approvedTargets[i]); } diff --git a/contracts/prebuilts/account/non-upgradeable/Account.sol b/contracts/prebuilts/account/non-upgradeable/Account.sol index 2152b85b4..09938c401 100644 --- a/contracts/prebuilts/account/non-upgradeable/Account.sol +++ b/contracts/prebuilts/account/non-upgradeable/Account.sol @@ -75,8 +75,10 @@ contract Account is AccountCore, ContractMetadata, ERC1271, ERC721Holder, ERC115 } address caller = msg.sender; + EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[signer]; + require( - _accountPermissionsStorage().approvedTargets[signer].contains(caller), + approvedTargets.contains(caller) || (approvedTargets.length() == 1 && approvedTargets.at(0) == address(0)), "Account: caller not approved target." ); diff --git a/contracts/prebuilts/account/utils/AccountCore.sol b/contracts/prebuilts/account/utils/AccountCore.sol index e52f8db89..c59cf8cac 100644 --- a/contracts/prebuilts/account/utils/AccountCore.sol +++ b/contracts/prebuilts/account/utils/AccountCore.sol @@ -76,6 +76,7 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc } /// @notice Returns whether a signer is authorized to perform transactions using the wallet. + /* solhint-disable*/ function isValidSigner(address _signer, UserOperation calldata _userOp) public view virtual returns (bool) { // First, check if the signer is an admin. if (_accountPermissionsStorage().isAdmin[_signer]) { @@ -83,12 +84,13 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc } SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[_signer]; + EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[_signer]; // If not an admin, check if the signer is active. if ( permissions.startTimestamp > block.timestamp || block.timestamp >= permissions.endTimestamp || - _accountPermissionsStorage().approvedTargets[_signer].length() == 0 + approvedTargets.length() == 0 ) { // Account: no active permissions. return false; @@ -97,15 +99,24 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc // Extract the function signature from the userOp calldata and check whether the signer is attempting to call `execute` or `executeBatch`. bytes4 sig = getFunctionSignature(_userOp.callData); + // if address(0) is the only approved target, set isWildCard to true (wildcard approved). + bool isWildCard = approvedTargets.length() == 1 && approvedTargets.at(0) == address(0); + if (sig == AccountExtension.execute.selector) { // Extract the `target` and `value` arguments from the calldata for `execute`. (address target, uint256 value) = decodeExecuteCalldata(_userOp.callData); + // if wildcard target is not approved, check that the target is in the approvedTargets set. + if (!isWildCard) { + // Check if the target is approved. + if (!approvedTargets.contains(target)) { + // Account: target not approved. + return false; + } + } + // Check if the value is within the allowed range and if the target is approved. - if ( - permissions.nativeTokenLimitPerTransaction < value || - !_accountPermissionsStorage().approvedTargets[_signer].contains(target) - ) { + if (permissions.nativeTokenLimitPerTransaction < value) { // Account: value too high OR Account: target not approved. return false; } @@ -113,12 +124,19 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc // Extract the `target` and `value` array arguments from the calldata for `executeBatch`. (address[] memory targets, uint256[] memory values, ) = decodeExecuteBatchCalldata(_userOp.callData); + // if wildcard target is not approved, check that the targets are in the approvedTargets set. + if (!isWildCard) { + for (uint256 i = 0; i < targets.length; i++) { + if (!approvedTargets.contains(targets[i])) { + // If any target is not approved, break the loop. + return false; + } + } + } + // For each target+value pair, check if the value is within the allowed range and if the target is approved. for (uint256 i = 0; i < targets.length; i++) { - if ( - permissions.nativeTokenLimitPerTransaction < values[i] || - !_accountPermissionsStorage().approvedTargets[_signer].contains(targets[i]) - ) { + if (permissions.nativeTokenLimitPerTransaction < values[i]) { // Account: value too high OR Account: target not approved. return false; } @@ -131,6 +149,8 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc return true; } + /* solhint-enable */ + /*/////////////////////////////////////////////////////////////// External functions //////////////////////////////////////////////////////////////*/ @@ -141,12 +161,14 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc } /// @notice Withdraw funds for this account from Entrypoint. - function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyAdmin { + function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public { + _onlyAdmin(); entryPoint().withdrawTo(withdrawAddress, amount); } /// @notice Overrides the Entrypoint contract being used. - function setEntrypointOverride(IEntryPoint _entrypointOverride) public virtual onlyAdmin { + function setEntrypointOverride(IEntryPoint _entrypointOverride) public virtual { + _onlyAdmin(); AccountCoreStorage.data().entrypointOverride = address(_entrypointOverride); } @@ -195,8 +217,10 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc if (!isValidSigner(signer, userOp)) return SIG_VALIDATION_FAILED; - uint48 validAfter = uint48(_accountPermissionsStorage().signerPermissions[signer].startTimestamp); - uint48 validUntil = uint48(_accountPermissionsStorage().signerPermissions[signer].endTimestamp); + SignerPermissionsStatic memory permissions = _accountPermissionsStorage().signerPermissions[signer]; + + uint48 validAfter = uint48(permissions.startTimestamp); + uint48 validUntil = uint48(permissions.endTimestamp); return _packValidationData(ValidationData(address(0), validAfter, validUntil)); } diff --git a/contracts/prebuilts/account/utils/AccountExtension.sol b/contracts/prebuilts/account/utils/AccountExtension.sol index 09c3fb2e5..161a124c1 100644 --- a/contracts/prebuilts/account/utils/AccountExtension.sol +++ b/contracts/prebuilts/account/utils/AccountExtension.sol @@ -77,8 +77,10 @@ contract AccountExtension is ContractMetadata, ERC1271, AccountPermissions, ERC7 } address caller = msg.sender; + EnumerableSet.AddressSet storage approvedTargets = _accountPermissionsStorage().approvedTargets[signer]; + require( - _accountPermissionsStorage().approvedTargets[signer].contains(caller), + approvedTargets.contains(caller) || (approvedTargets.length() == 1 && approvedTargets.at(0) == address(0)), "Account: caller not approved target." ); diff --git a/src/test/smart-wallet/AccountVulnPOC.t.sol b/src/test/smart-wallet/AccountVulnPOC.t.sol index 28e5b877c..4de05daf2 100644 --- a/src/test/smart-wallet/AccountVulnPOC.t.sol +++ b/src/test/smart-wallet/AccountVulnPOC.t.sol @@ -243,7 +243,7 @@ contract SimpleAccountVulnPOCTest is BaseTest { Setup //////////////////////////////////////////////////////////////*/ address[] memory approvedTargets = new address[](1); - approvedTargets[0] = address(0); // allowing accountSigner permissions for some random contract, consider it as 0 address here + approvedTargets[0] = address(0x123); // allowing accountSigner permissions for some random contract, consider it as 0 address here IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest( accountSigner, diff --git a/src/test/smart-wallet/account-core/isValidSigner.t.sol b/src/test/smart-wallet/account-core/isValidSigner.t.sol new file mode 100644 index 000000000..453a8a630 --- /dev/null +++ b/src/test/smart-wallet/account-core/isValidSigner.t.sol @@ -0,0 +1,562 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +// Test utils +import { BaseTest } from "../../utils/BaseTest.sol"; +import "contracts/external-deps/openzeppelin/proxy/Clones.sol"; +import "@thirdweb-dev/dynamic-contracts/src/interface/IExtension.sol"; +import { IAccountPermissions } from "contracts/extension/interface/IAccountPermissions.sol"; +import { AccountPermissions, EnumerableSet, ECDSA } from "contracts/extension/upgradeable/AccountPermissions.sol"; + +// Account Abstraction setup for smart wallets. +import { EntryPoint, IEntryPoint } from "contracts/prebuilts/account/utils/Entrypoint.sol"; +import { UserOperation } from "contracts/prebuilts/account/utils/UserOperation.sol"; + +// Target +import { DynamicAccountFactory, DynamicAccount, BaseAccountFactory } from "contracts/prebuilts/account/dynamic/DynamicAccountFactory.sol"; + +/// @dev This is a dummy contract to test contract interactions with Account. +contract Number { + uint256 public num; + + function setNum(uint256 _num) public { + num = _num; + } + + function doubleNum() public { + num *= 2; + } + + function incrementNum() public { + num += 1; + } +} + +contract MyDynamicAccount is DynamicAccount { + using EnumerableSet for EnumerableSet.AddressSet; + + constructor(IEntryPoint _entrypoint, Extension[] memory _defaultExtensions) + DynamicAccount(_entrypoint, _defaultExtensions) + {} + + function setPermissionsForSigner( + address _signer, + uint256 _nativeTokenLimit, + uint256 _startTimestamp, + uint256 _endTimestamp + ) public { + _accountPermissionsStorage().signerPermissions[_signer] = SignerPermissionsStatic( + _nativeTokenLimit, + uint128(_startTimestamp), + uint128(_endTimestamp) + ); + } + + function setApprovedTargetsForSigner(address _signer, address[] memory _approvedTargets) public { + uint256 len = _approvedTargets.length; + for (uint256 i = 0; i < len; i += 1) { + _accountPermissionsStorage().approvedTargets[_signer].add(_approvedTargets[i]); + } + } + + function _setAdmin(address _account, bool _isAdmin) internal virtual override { + _accountPermissionsStorage().isAdmin[_account] = _isAdmin; + } + + function _isAuthorizedCallToUpgrade() internal view virtual override returns (bool) {} +} + +contract AccountCoreTest_isValidSigner is BaseTest { + // Target contracts + EntryPoint private entrypoint; + DynamicAccountFactory private accountFactory; + MyDynamicAccount private account; + + // Mocks + Number internal numberContract; + + // Test params + uint256 private accountAdminPKey = 100; + address private accountAdmin; + + uint256 private accountSignerPKey = 200; + address private accountSigner; + + uint256 private nonSignerPKey = 300; + address private nonSigner; + + address private opSigner; + uint256 private startTimestamp; + uint256 private endTimestamp; + uint256 private nativeTokenLimit; + UserOperation private op; + + bytes internal data = bytes(""); + + function _setupUserOp( + uint256 _signerPKey, + bytes memory _initCode, + bytes memory _callDataForEntrypoint + ) internal returns (UserOperation memory) { + uint256 nonce = entrypoint.getNonce(address(account), 0); + + // Get user op fields + UserOperation memory op = UserOperation({ + sender: address(account), + nonce: nonce, + initCode: _initCode, + callData: _callDataForEntrypoint, + callGasLimit: 5_000_000, + verificationGasLimit: 5_000_000, + preVerificationGas: 5_000_000, + maxFeePerGas: 0, + maxPriorityFeePerGas: 0, + paymasterAndData: bytes(""), + signature: bytes("") + }); + + // Sign UserOp + bytes32 opHash = EntryPoint(entrypoint).getUserOpHash(op); + bytes32 msgHash = ECDSA.toEthSignedMessageHash(opHash); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPKey, msgHash); + bytes memory userOpSignature = abi.encodePacked(r, s, v); + + address recoveredSigner = ECDSA.recover(msgHash, v, r, s); + address expectedSigner = vm.addr(_signerPKey); + assertEq(recoveredSigner, expectedSigner); + + op.signature = userOpSignature; + + return op; + } + + function _setupUserOpExecute( + uint256 _signerPKey, + bytes memory _initCode, + address _target, + uint256 _value, + bytes memory _callData + ) internal returns (UserOperation memory) { + bytes memory callDataForEntrypoint = abi.encodeWithSignature( + "execute(address,uint256,bytes)", + _target, + _value, + _callData + ); + + return _setupUserOp(_signerPKey, _initCode, callDataForEntrypoint); + } + + function _setupUserOpExecuteBatch( + uint256 _signerPKey, + bytes memory _initCode, + address[] memory _targets, + uint256[] memory _values, + bytes[] memory _callData + ) internal returns (UserOperation memory) { + bytes memory callDataForEntrypoint = abi.encodeWithSignature( + "executeBatch(address[],uint256[],bytes[])", + _targets, + _values, + _callData + ); + + return _setupUserOp(_signerPKey, _initCode, callDataForEntrypoint); + } + + function _setupUserOpInvalidFunction(uint256 _signerPKey, bytes memory _initCode) + internal + returns (UserOperation memory) + { + bytes memory callDataForEntrypoint = abi.encodeWithSignature("invalidFunction()"); + + return _setupUserOp(_signerPKey, _initCode, callDataForEntrypoint); + } + + function setUp() public override { + super.setUp(); + + // Setup signers. + accountAdmin = vm.addr(accountAdminPKey); + vm.deal(accountAdmin, 100 ether); + + accountSigner = vm.addr(accountSignerPKey); + nonSigner = vm.addr(nonSignerPKey); + + // Setup contracts + entrypoint = new EntryPoint(); + + IExtension.Extension[] memory extensions; + + // deploy account factory + accountFactory = new DynamicAccountFactory(IEntryPoint(payable(address(entrypoint))), extensions); + // deploy dummy contract + numberContract = new Number(); + + address accountImpl = address(new MyDynamicAccount(IEntryPoint(payable(address(entrypoint))), extensions)); + address _account = Clones.cloneDeterministic(accountImpl, "salt"); + account = MyDynamicAccount(payable(_account)); + account.initialize(accountAdmin, ""); + } + + function test_isValidSigner_whenSignerIsAdmin() public { + opSigner = accountAdmin; + UserOperation memory _op; // empty op since it's not relevant for this check + bool isValid = DynamicAccount(payable(account)).isValidSigner(opSigner, _op); + + assertTrue(isValid); + } + + modifier whenNotAdmin() { + opSigner = accountSigner; + _; + } + + function test_isValidSigner_invalidTimestamps() public whenNotAdmin { + UserOperation memory _op; // empty op since it's not relevant for this check + startTimestamp = 100; + endTimestamp = 200; + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + vm.warp(201); // block timestamp greater than end timestamp + bool isValid = account.isValidSigner(opSigner, _op); + + assertFalse(isValid); + + vm.warp(200); // block timestamp equal to end timestamp + isValid = account.isValidSigner(opSigner, _op); + + assertFalse(isValid); + + vm.warp(99); // block timestamp less than start timestamp + isValid = account.isValidSigner(opSigner, _op); + + assertFalse(isValid); + } + + modifier whenValidTimestamps() { + startTimestamp = 100; + endTimestamp = 200; + vm.warp(150); // block timestamp within start and end timestamps + _; + } + + function test_isValidSigner_noApprovedTargets() public whenNotAdmin whenValidTimestamps { + UserOperation memory _op; // empty op since it's not relevant for this check + address[] memory _approvedTargets; + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + bool isValid = account.isValidSigner(opSigner, _op); + + assertFalse(isValid); + } + + // ================== + // ======= Test branch: wildcard + // ================== + + function test_isValidSigner_wildcardExecute_breachNativeTokenLimit() public whenNotAdmin whenValidTimestamps { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(0); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpExecute(accountSignerPKey, bytes(""), address(0x123), 10, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + function test_isValidSigner_wildcardExecuteBatch_breachNativeTokenLimit() public whenNotAdmin whenValidTimestamps { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(0); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + uint256 count = 3; + address[] memory targets = new address[](count); + uint256[] memory values = new uint256[](count); + bytes[] memory callData = new bytes[](count); + + for (uint256 i = 0; i < count; i += 1) { + targets[i] = address(numberContract); + values[i] = 10; + callData[i] = abi.encodeWithSignature("incrementNum()", i); + } + + op = _setupUserOpExecuteBatch(accountSignerPKey, bytes(""), targets, values, callData); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + modifier whenWithinNativeTokenLimit() { + nativeTokenLimit = 1000; + _; + } + + function test_isValidSigner_wildcardExecute() public whenNotAdmin whenValidTimestamps whenWithinNativeTokenLimit { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(0); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpExecute(accountSignerPKey, bytes(""), address(0x123), 10, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertTrue(isValid); + } + + function test_isValidSigner_wildcardExecuteBatch() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(0); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + uint256 count = 3; + address[] memory targets = new address[](count); + uint256[] memory values = new uint256[](count); + bytes[] memory callData = new bytes[](count); + + for (uint256 i = 0; i < count; i += 1) { + targets[i] = address(numberContract); + values[i] = 10; + callData[i] = abi.encodeWithSignature("incrementNum()", i); + } + + op = _setupUserOpExecuteBatch(accountSignerPKey, bytes(""), targets, values, callData); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertTrue(isValid); + } + + function test_isValidSigner_wildcardInvalidFunction() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(0); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpInvalidFunction(accountSignerPKey, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + // ================== + // ======= Test branch: not wildcard + // ================== + + function test_isValidSigner_execute_callingWrongTarget() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + address wrongTarget = address(0x123); + op = _setupUserOpExecute(accountSignerPKey, bytes(""), wrongTarget, 10, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + function test_isValidSigner_executeBatch_callingWrongTarget() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + uint256 count = 3; + address[] memory targets = new address[](count); + uint256[] memory values = new uint256[](count); + bytes[] memory callData = new bytes[](count); + address wrongTarget = address(0x123); + for (uint256 i = 0; i < count; i += 1) { + targets[i] = wrongTarget; + values[i] = 10; + callData[i] = abi.encodeWithSignature("incrementNum()", i); + } + + op = _setupUserOpExecuteBatch(accountSignerPKey, bytes(""), targets, values, callData); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + modifier whenCorrectTarget() { + _; + } + + function test_isValidSigner_execute_breachNativeTokenLimit() + public + whenNotAdmin + whenValidTimestamps + whenCorrectTarget + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpExecute(accountSignerPKey, bytes(""), address(numberContract), 10, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + function test_isValidSigner_executeBatch_breachNativeTokenLimit() + public + whenNotAdmin + whenValidTimestamps + whenCorrectTarget + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + uint256 count = 3; + address[] memory targets = new address[](count); + uint256[] memory values = new uint256[](count); + bytes[] memory callData = new bytes[](count); + + for (uint256 i = 0; i < count; i += 1) { + targets[i] = address(numberContract); + values[i] = 10; + callData[i] = abi.encodeWithSignature("incrementNum()", i); + } + + op = _setupUserOpExecuteBatch(accountSignerPKey, bytes(""), targets, values, callData); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } + + function test_isValidSigner_execute() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + whenCorrectTarget + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpExecute(accountSignerPKey, bytes(""), address(numberContract), 10, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertTrue(isValid); + } + + function test_isValidSigner_executeBatch() + public + whenNotAdmin + whenValidTimestamps + whenWithinNativeTokenLimit + whenCorrectTarget + { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + uint256 count = 3; + address[] memory targets = new address[](count); + uint256[] memory values = new uint256[](count); + bytes[] memory callData = new bytes[](count); + + for (uint256 i = 0; i < count; i += 1) { + targets[i] = address(numberContract); + values[i] = 10; + callData[i] = abi.encodeWithSignature("incrementNum()", i); + } + + op = _setupUserOpExecuteBatch(accountSignerPKey, bytes(""), targets, values, callData); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertTrue(isValid); + } + + function test_isValidSigner_invalidFunction() public whenNotAdmin whenValidTimestamps whenWithinNativeTokenLimit { + // set wildcard + address[] memory _approvedTargets = new address[](1); + _approvedTargets[0] = address(numberContract); + account.setApprovedTargetsForSigner(opSigner, _approvedTargets); + + // user op execute + op = _setupUserOpInvalidFunction(accountSignerPKey, bytes("")); + + account.setPermissionsForSigner(opSigner, nativeTokenLimit, startTimestamp, endTimestamp); + + bool isValid = account.isValidSigner(opSigner, op); + + assertFalse(isValid); + } +} diff --git a/src/test/smart-wallet/account-core/isValidSigner.tree b/src/test/smart-wallet/account-core/isValidSigner.tree new file mode 100644 index 000000000..cc4497260 --- /dev/null +++ b/src/test/smart-wallet/account-core/isValidSigner.tree @@ -0,0 +1,46 @@ +isValidSigner(address _signer, UserOperation calldata _userOp) +├── when `_signer` is admin +│ └── it should return true +├── when `_signer` is not admin + └── when timestamp is invalid + │ └── it should return false + └── when timestamp is valid + └── when no approved targets + │ └── it should return false + │ + │ // Case - Wildcard + └── when approved targets length is equal to 1 and contains address(0) + │ └── when calling `execute` function + │ │ └── when the decoded `value` is more than nativeTokenLimitPerTransaction + │ │ │ └── it should return false + │ │ └── when the decoded `value` is within nativeTokenLimitPerTransaction + │ │ └── it should return true + │ └── when calling `batchExecute` function + │ │ └── when any item in the decoded `values` array is more than nativeTokenLimitPerTransaction + │ │ │ └── it should return false + │ │ └── when all items in the decoded `values` array are within nativeTokenLimitPerTransaction + │ │ └── it should return true + │ └── when calling an invalid function + │ └── it should return false + │ + │ // Case - No Wildcard + └── when approved targets length is greater than 1, or doesn't contain address(0) + └── when calling `execute` function + │ └── when approvedTargets doesn't contain the decoded `target` + │ │ └── it should return false + │ └── when approvedTargets contains the decoded `target` + │ └── when the decoded `value` is more than nativeTokenLimitPerTransaction + │ │ └── it should return false + │ └── when the decoded `value` is within nativeTokenLimitPerTransaction + │ └── it should return true + └── when calling `batchExecute` function + │ └── when approvedTargets doesn't contain one or more items in the decoded `targets` array + │ │ └── it should return false + │ └── when approvedTargets contains all items in the decdoded `targets` array + │ └── when any item in the decoded `values` array is more than nativeTokenLimitPerTransaction + │ │ └── it should return false + │ └── when all items in the decoded `values` array are within nativeTokenLimitPerTransaction + │ └── it should return true + └── when calling an invalid function + └── it should return false + \ No newline at end of file diff --git a/src/test/smart-wallet/account-permissions/setPermissionsForSigner.t.sol b/src/test/smart-wallet/account-permissions/setPermissionsForSigner.t.sol index db8e9b09a..170db6fcd 100644 --- a/src/test/smart-wallet/account-permissions/setPermissionsForSigner.t.sol +++ b/src/test/smart-wallet/account-permissions/setPermissionsForSigner.t.sol @@ -323,18 +323,17 @@ contract AccountPermissionsTest_setPermissionsForSigner is BaseTest { address[] memory approvedTargets = new address[](0); { - IAccountPermissions.SignerPermissionRequest memory request = IAccountPermissions - .SignerPermissionRequest( - accountSigner, - 1, - approvedTargets, - 1, - 0, - type(uint128).max, - 0, - type(uint128).max, - uidCache - ); + IAccountPermissions.SignerPermissionRequest memory request = IAccountPermissions.SignerPermissionRequest( + accountSigner, + 1, + approvedTargets, + 1, + 0, + type(uint128).max, + 0, + type(uint128).max, + uidCache + ); bytes memory sig2 = _signSignerPermissionRequest(request); SimpleAccount(payable(account)).setPermissionsForSigner(request, sig2); @@ -541,18 +540,17 @@ contract AccountPermissionsTest_setPermissionsForSigner is BaseTest { { //set admin status - IAccountPermissions.SignerPermissionRequest memory req = IAccountPermissions - .SignerPermissionRequest( - accountSigner, - 1, - approvedTargets, - 0, - 0, - type(uint128).max, - 0, - type(uint128).max, - uidCache - ); + IAccountPermissions.SignerPermissionRequest memory req = IAccountPermissions.SignerPermissionRequest( + accountSigner, + 1, + approvedTargets, + 0, + 0, + type(uint128).max, + 0, + type(uint128).max, + uidCache + ); vm.prank(accountAdmin); bytes memory sig2 = _signSignerPermissionRequest(req); @@ -577,7 +575,7 @@ contract AccountPermissionsTest_setPermissionsForSigner is BaseTest { vm.prank(accountAdmin); bytes memory sig3 = _signSignerPermissionRequest(permissionsReq); - vm.expectRevert("already admin"); + vm.expectRevert("admin"); SimpleAccount(payable(account)).setPermissionsForSigner(permissionsReq, sig3); }