Skip to content

Commit

Permalink
add wildcard target to isValidSigner/isValidSignature (#539)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
Co-authored-by: Yash <[email protected]>
  • Loading branch information
@WhiteOakKong @joaquim-verges @kumaryash90
3 people authored Oct 13, 2023
1 parent 4161cab commit 498d7dd
Show file tree
Hide file tree
Showing 8 changed files with 680 additions and 47 deletions.
11 changes: 5 additions & 6 deletions contracts/extension/upgradeable/AccountPermissions.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
_;
}

/*///////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -78,7 +77,7 @@ abstract contract AccountPermissions is IAccountPermissions, EIP712 {
return;
}

require(!isAdmin(targetSigner), "already admin");
require(!isAdmin(targetSigner), "admin");

_accountPermissionsStorage().allSigners.add(targetSigner);

Expand All @@ -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]);
}
Expand Down
4 changes: 3 additions & 1 deletion contracts/prebuilts/account/non-upgradeable/Account.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."
);

Expand Down
50 changes: 37 additions & 13 deletions contracts/prebuilts/account/utils/AccountCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,21 @@ 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]) {
return true;
}

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;
Expand All @@ -97,28 +99,44 @@ 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;
}
} else if (sig == AccountExtension.executeBatch.selector) {
// 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;
}
Expand All @@ -131,6 +149,8 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, Acc
return true;
}

/* solhint-enable */

/*///////////////////////////////////////////////////////////////
External functions
//////////////////////////////////////////////////////////////*/
Expand All @@ -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);
}

Expand Down Expand Up @@ -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));
}
Expand Down
4 changes: 3 additions & 1 deletion contracts/prebuilts/account/utils/AccountExtension.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."
);

Expand Down
2 changes: 1 addition & 1 deletion src/test/smart-wallet/AccountVulnPOC.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 498d7dd

Please sign in to comment.