Skip to content

Commit

Permalink
(H-2) Arbitrary code execution facilitated by isApprovedForFunction /…
Browse files Browse the repository at this point in the history 
… approveSignerForFunction
  • Loading branch information
@kumaryash90
kumaryash90 committed Jan 22, 2023
1 parent 3cbdf62 commit 20ddc9e
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 173 deletions.
36 changes: 1 addition & 35 deletions contracts/thirdweb-wallet/Account.sol
View file
Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -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.");
Expand Down
22 changes: 0 additions & 22 deletions contracts/thirdweb-wallet/interface/IAccount.sol
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
Expand All @@ -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);
}
116 changes: 0 additions & 116 deletions src/test/thirdweb-wallet/AccountTest.t.sol
View file
Expand Up @@ -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.
//////////////////////////////////////////////////////////////*/
Expand Down

0 comments on commit 20ddc9e

Please sign in to comment.