From 20ddc9eeacecaa837ad1417002a97a8632537637 Mon Sep 17 00:00:00 2001 From: Yash Date: Mon, 23 Jan 2023 00:22:18 +0530 Subject: [PATCH] (H-2) Arbitrary code execution facilitated by isApprovedForFunction / approveSignerForFunction --- contracts/thirdweb-wallet/Account.sol | 36 +----- .../thirdweb-wallet/interface/IAccount.sol | 22 ---- src/test/thirdweb-wallet/AccountTest.t.sol | 116 ------------------ 3 files changed, 1 insertion(+), 173 deletions(-) diff --git a/contracts/thirdweb-wallet/Account.sol b/contracts/thirdweb-wallet/Account.sol index d4b86f038..4f979a7ab 100644 --- a/contracts/thirdweb-wallet/Account.sol +++ b/contracts/thirdweb-wallet/Account.sol @@ -81,18 +81,12 @@ contract Account is /// @notice Mapping from Signer => contracts approved to call. mapping(address => EnumerableSet.AddressSet) private approvedContracts; - /// @notice Mapping from Signer => functions approved to call. - mapping(address => EnumerableSet.Bytes32Set) private approvedFunctions; - /// @notice Mapping from Signer => (fn sig, contract address) => approval to call. mapping(address => mapping(bytes32 => bool)) private isApprovedFor; /// @notice Mapping from Signer => contract address => approval to call. mapping(address => mapping(address => bool)) private isApprovedForContract; - /// @notice Mapping from Signer => function signature => approval to call. - mapping(address => mapping(bytes4 => bool)) private isApprovedForFunction; - /*/////////////////////////////////////////////////////////////// Constructor + initializer //////////////////////////////////////////////////////////////*/ @@ -277,14 +271,6 @@ contract Account is emit ContractApprovedForSigner(_signer, _target, true); } - /// @notice Approves a signer to be able to call `_selector` function on any smart contract. - function approveSignerForFunction(address _signer, bytes4 _selector) external onlySelf { - require(approvedFunctions[_signer].add(bytes32(_selector)), "Account: already approved."); - isApprovedForFunction[_signer][_selector] = true; - - emit FunctionApprovedForSigner(_signer, _selector, true); - } - /// @notice Removes approval of a signer from being able to call `_selector` function on `_target` smart contract. function disapproveSignerForTarget( address _signer, @@ -320,14 +306,6 @@ contract Account is emit ContractApprovedForSigner(_signer, _target, false); } - /// @notice Disapproves a signer from being able to call `_selector` function on arbitrary smart contract. - function disapproveSignerForFunction(address _signer, bytes4 _selector) external onlySelf { - require(approvedFunctions[_signer].remove(bytes32(_selector)), "Account: already not approved."); - isApprovedForFunction[_signer][_selector] = false; - - emit FunctionApprovedForSigner(_signer, _selector, false); - } - /// @notice Returns all call targets approved for a given signer. function getAllApprovedTargets(address _signer) external view returns (CallTarget[] memory approvedTargets) { CallTarget[] memory targets = callTargets[_signer]; @@ -358,16 +336,6 @@ contract Account is return approvedContracts[_signer].values(); } - /// @notice Returns all function targets approved for a given signer. - function getAllApprovedFunctions(address _signer) external view returns (bytes4[] memory functions) { - uint256 len = approvedFunctions[_signer].length(); - functions = new bytes4[](len); - - for (uint256 i = 0; i < len; i += 1) { - functions[i] = bytes4(approvedFunctions[_signer].at(i)); - } - } - /*/////////////////////////////////////////////////////////////// EIP-1271 Smart contract signatures //////////////////////////////////////////////////////////////*/ @@ -450,9 +418,7 @@ contract Account is bytes32 targetHash = keccak256(abi.encode(_getSelector(_data), _target)); hasPermissions = hasRole(SIGNER_ROLE, _signer) && - (isApprovedFor[_signer][targetHash] || - isApprovedForContract[_signer][_target] || - isApprovedForFunction[_signer][_getSelector(_data)]); + (isApprovedFor[_signer][targetHash] || isApprovedForContract[_signer][_target]); } require(hasPermissions, "Account: unauthorized signer."); diff --git a/contracts/thirdweb-wallet/interface/IAccount.sol b/contracts/thirdweb-wallet/interface/IAccount.sol index 141ba63b7..8ca791be9 100644 --- a/contracts/thirdweb-wallet/interface/IAccount.sol +++ b/contracts/thirdweb-wallet/interface/IAccount.sol @@ -135,9 +135,6 @@ interface IAccount is IERC1271 { /// @notice Emitted when a signer is approved to call arbitrary function on `target` smart contract. event ContractApprovedForSigner(address indexed signer, address indexed targetContract, bool approval); - /// @notice Emitted when a signer is approved to call `selector` function on arbitrary smart contract. - event FunctionApprovedForSigner(address indexed signer, bytes4 indexed selector, bool approval); - /// @notice A struct representing a call target (fn selector + smart contract). struct CallTarget { bytes4 selector; @@ -165,14 +162,6 @@ interface IAccount is IERC1271 { */ function approveSignerForContract(address signer, address target) external; - /** - * @notice Approves a signer to be able to call `selector` function on any smart contract. - * - * @param signer The signer to approve. - * @param selector The function selector to approve the signer for. - */ - function approveSignerForFunction(address signer, bytes4 selector) external; - /** * @notice Removes approval of a signer from being able to call `_selector` function on `_target` smart contract. * @@ -194,20 +183,9 @@ interface IAccount is IERC1271 { */ function disapproveSignerForContract(address signer, address target) external; - /** - * @notice Disapproves a signer from being able to call `_selector` function on arbitrary smart contract. - * - * @param signer The signer to remove approval for. - * @param selector The function selector for which to remove the approval of the signer. - */ - function disapproveSignerForFunction(address signer, bytes4 selector) external; - /// @notice Returns all call targets approved for a given signer. function getAllApprovedTargets(address signer) external view returns (CallTarget[] memory approvedTargets); /// @notice Returns all contract targets approved for a given signer. function getAllApprovedContracts(address signer) external view returns (address[] memory contracts); - - /// @notice Returns all function targets approved for a given signer. - function getAllApprovedFunctions(address signer) external view returns (bytes4[] memory functions); } diff --git a/src/test/thirdweb-wallet/AccountTest.t.sol b/src/test/thirdweb-wallet/AccountTest.t.sol index 0224341c5..72264ab72 100644 --- a/src/test/thirdweb-wallet/AccountTest.t.sol +++ b/src/test/thirdweb-wallet/AccountTest.t.sol @@ -862,122 +862,6 @@ contract ThirdwebWalletTest is BaseTest, AccountUtil, AccountData, AccountAdminU accountAdmin.relay(admin, adminAccountId, 0, 0, data); } - /*/////////////////////////////////////////////////////////////// - Test action: Admin approves non-admin signer for function. - //////////////////////////////////////////////////////////////*/ - - function test_state_execute_approveSignerForFunction() external { - _setUp_NonAdminForAccount(); - - ////////// approve signer for target ////////// - - bytes memory dataToRelay = abi.encodeWithSelector( - Account.approveSignerForFunction.selector, - nonAdmin, - CounterContract.setNumber.selector - ); - - IAccount.TransactionParams memory params = IAccount.TransactionParams({ - signer: admin, - target: address(account), - data: dataToRelay, - nonce: account.nonce(), - value: 0, - gas: 0, - validityStartTimestamp: 0, - validityEndTimestamp: 100 - }); - bytes memory signature = signExecute(params, privateKey1, address(account)); - - bytes memory data = abi.encodeWithSelector(Account.execute.selector, params, signature); - accountAdmin.relay(admin, adminAccountId, 0, 0, data); - - ////////// signer calls one contract ////////// - - uint256 number = 5; - uint256 currentNonce = account.nonce(); - assertEq(counter.getNumber(), 0); - - IAccount.TransactionParams memory params2 = IAccount.TransactionParams({ - signer: nonAdmin, - target: address(counter), - data: abi.encodeWithSelector(CounterContract.setNumber.selector, number), - nonce: account.nonce(), - value: 0, - gas: 0, - validityStartTimestamp: 0, - validityEndTimestamp: 100 - }); - bytes memory signature2 = signExecute(params2, privateKey2, address(account)); // nonAdmin signs - - bytes memory data2 = abi.encodeWithSelector(Account.execute.selector, params2, signature2); - accountAdmin.relay(nonAdmin, nonAdminAccountId, 0, 0, data2); - - assertEq(account.nonce(), currentNonce + 1); - assertEq(counter.getNumber(), number); - - ////////// signer calls another contract ////////// - - CounterContract newCounter = new CounterContract(); - - currentNonce = account.nonce(); - assertEq(newCounter.getNumber(), 0); - - IAccount.TransactionParams memory params3 = IAccount.TransactionParams({ - signer: nonAdmin, - target: address(newCounter), - data: abi.encodeWithSelector(CounterContract.setNumber.selector, number), - nonce: account.nonce(), - value: 0, - gas: 0, - validityStartTimestamp: 0, - validityEndTimestamp: 100 - }); - bytes memory signature3 = signExecute(params3, privateKey2, address(account)); // nonAdmin signs - - bytes memory data3 = abi.encodeWithSelector(Account.execute.selector, params3, signature3); - accountAdmin.relay(nonAdmin, nonAdminAccountId, 0, 0, data3); - - assertEq(account.nonce(), currentNonce + 1); - assertEq(newCounter.getNumber(), number); - } - - function test_revert_execute_approveSignerForFunction_alreadyApproved() external { - _setUp_NonAdminForAccount(); - - ////////// approve signer for target ////////// - - bytes memory dataToRelay = abi.encodeWithSelector( - Account.approveSignerForFunction.selector, - nonAdmin, - CounterContract.setNumber.selector - ); - - IAccount.TransactionParams memory params = IAccount.TransactionParams({ - signer: admin, - target: address(account), - data: dataToRelay, - nonce: account.nonce(), - value: 0, - gas: 0, - validityStartTimestamp: 0, - validityEndTimestamp: 100 - }); - bytes memory signature = signExecute(params, privateKey1, address(account)); - - bytes memory data = abi.encodeWithSelector(Account.execute.selector, params, signature); - accountAdmin.relay(admin, adminAccountId, 0, 0, data); - - ////////// signer already approved for target ////////// - - params.nonce = account.nonce(); - signature = signExecute(params, privateKey, address(account)); - - data = abi.encodeWithSelector(Account.execute.selector, params, signature); - vm.expectRevert("Account: already approved."); - accountAdmin.relay(admin, adminAccountId, 0, 0, data); - } - /*/////////////////////////////////////////////////////////////// Test action: Admin adds another admin to account. //////////////////////////////////////////////////////////////*/