From 8f35cb2f8aa770f961d0a1a1cefe7f209bdda285 Mon Sep 17 00:00:00 2001 From: Krishang Date: Thu, 17 Aug 2023 17:51:11 -0400 Subject: [PATCH] Fix vuln: eip-1271 caller must be approved target --- .../smart-wallet/non-upgradeable/Account.sol | 10 +- contracts/smart-wallet/utils/AccountCore.sol | 10 +- src/test/smart-wallet/AccountVulnPOC.t.sol | 281 ++++++++++++++++++ 3 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 src/test/smart-wallet/AccountVulnPOC.t.sol diff --git a/contracts/smart-wallet/non-upgradeable/Account.sol b/contracts/smart-wallet/non-upgradeable/Account.sol index a20f100a0..5865f66a8 100644 --- a/contracts/smart-wallet/non-upgradeable/Account.sol +++ b/contracts/smart-wallet/non-upgradeable/Account.sol @@ -165,7 +165,15 @@ contract Account is { address signer = _hash.recover(_signature); - if (isAdmin(signer) || isActiveSigner(signer)) { + if (isAdmin(signer)) { + return MAGICVALUE; + } + + AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage(); + address caller = msg.sender; + require(data.approvedTargets[signer].contains(caller), "Account: caller not approved target."); + + if (isActiveSigner(signer)) { magicValue = MAGICVALUE; } } diff --git a/contracts/smart-wallet/utils/AccountCore.sol b/contracts/smart-wallet/utils/AccountCore.sol index b11c60cf3..7b7e81ab4 100644 --- a/contracts/smart-wallet/utils/AccountCore.sol +++ b/contracts/smart-wallet/utils/AccountCore.sol @@ -143,7 +143,15 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, ERC { address signer = _hash.recover(_signature); - if (isAdmin(signer) || isActiveSigner(signer)) { + if (isAdmin(signer)) { + return MAGICVALUE; + } + + AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage(); + address caller = msg.sender; + require(data.approvedTargets[signer].contains(caller), "Account: caller not approved target."); + + if (isActiveSigner(signer)) { magicValue = MAGICVALUE; } } diff --git a/src/test/smart-wallet/AccountVulnPOC.t.sol b/src/test/smart-wallet/AccountVulnPOC.t.sol new file mode 100644 index 000000000..eaeef0a4b --- /dev/null +++ b/src/test/smart-wallet/AccountVulnPOC.t.sol @@ -0,0 +1,281 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +// Test utils +import "../utils/BaseTest.sol"; +// Account Abstraction setup for smart wallets. +import { EntryPoint, IEntryPoint } from "contracts/smart-wallet/utils/Entrypoint.sol"; +import { UserOperation } from "contracts/smart-wallet/utils/UserOperation.sol"; + +// Target +import { IAccountPermissions } from "contracts/extension/interface/IAccountPermissions.sol"; +import { AccountFactory, Account } from "contracts/smart-wallet/non-upgradeable/AccountFactory.sol"; + +library GPv2EIP1271 { + bytes4 internal constant MAGICVALUE = 0x1626ba7e; +} + +interface EIP1271Verifier { + function isValidSignature(bytes32 _hash, bytes memory _signature) external view returns (bytes4 magicValue); +} + +/// @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; + } + + function setNumBySignature( + address owner, + uint256 newNum, + bytes calldata signature + ) public { + if (owner.code.length == 0) { + // Signature verification by ECDSA + } else { + // Signature verfication by EIP1271 + bytes32 digest = keccak256(abi.encode(newNum)); + require( + EIP1271Verifier(owner).isValidSignature(digest, signature) == GPv2EIP1271.MAGICVALUE, + "GPv2: invalid eip1271 signature" + ); + num = newNum; + } + } +} + +contract SimpleAccountVulnPOCTest is BaseTest { + // Target contracts + EntryPoint private entrypoint; + AccountFactory private accountFactory; + + // 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; + + // UserOp terminology: `sender` is the smart wallet. + address private sender = 0xBB956D56140CA3f3060986586A2631922a4B347E; + address payable private beneficiary = payable(address(0x45654)); + + bytes32 private uidCache = bytes32("random uid"); + + event AccountCreated(address indexed account, address indexed accountAdmin); + + function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req) + internal + view + returns (bytes memory signature) + { + bytes32 typehashSignerPermissionRequest = keccak256( + "SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)" + ); + bytes32 nameHash = keccak256(bytes("Account")); + bytes32 versionHash = keccak256(bytes("1")); + bytes32 typehashEip712 = keccak256( + "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" + ); + bytes32 domainSeparator = keccak256(abi.encode(typehashEip712, nameHash, versionHash, block.chainid, sender)); + + bytes memory encodedRequest = abi.encode( + typehashSignerPermissionRequest, + _req.signer, + keccak256(abi.encodePacked(_req.approvedTargets)), + _req.nativeTokenLimitPerTransaction, + _req.permissionStartTimestamp, + _req.permissionEndTimestamp, + _req.reqValidityStartTimestamp, + _req.reqValidityEndTimestamp, + _req.uid + ); + bytes32 structHash = keccak256(encodedRequest); + bytes32 typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(accountAdminPKey, typedDataHash); + signature = abi.encodePacked(r, s, v); + } + + function _setupUserOp( + uint256 _signerPKey, + bytes memory _initCode, + bytes memory _callDataForEntrypoint + ) internal returns (UserOperation[] memory ops) { + uint256 nonce = entrypoint.getNonce(sender, 0); + + // Get user op fields + UserOperation memory op = UserOperation({ + sender: sender, + nonce: nonce, + initCode: _initCode, + callData: _callDataForEntrypoint, + callGasLimit: 500_000, + verificationGasLimit: 500_000, + preVerificationGas: 500_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; + + // Store UserOp + ops = new UserOperation[](1); + ops[0] = 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 _target, + uint256[] memory _value, + bytes[] memory _callData + ) internal returns (UserOperation[] memory) { + bytes memory callDataForEntrypoint = abi.encodeWithSignature( + "executeBatch(address[],uint256[],bytes[])", + _target, + _value, + _callData + ); + + 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(); + // deploy account factory + accountFactory = new AccountFactory(IEntryPoint(payable(address(entrypoint)))); + // deploy dummy contract + numberContract = new Number(); + } + + /*////////////////////////////////////////////////////////// + Test: performing a contract call + //////////////////////////////////////////////////////////////*/ + + function _setup_executeTransaction() internal { + bytes memory initCallData = abi.encodeWithSignature("createAccount(address,bytes)", accountAdmin, bytes("")); + bytes memory initCode = abi.encodePacked(abi.encodePacked(address(accountFactory)), initCallData); + + UserOperation[] memory userOpCreateAccount = _setupUserOpExecute( + accountAdminPKey, + initCode, + address(0), + 0, + bytes("") + ); + + EntryPoint(entrypoint).handleOps(userOpCreateAccount, beneficiary); + } + + function test_POC() public { + _setup_executeTransaction(); + + /*////////////////////////////////////////////////////////// + Setup + //////////////////////////////////////////////////////////////*/ + address[] memory approvedTargets = new address[](1); + approvedTargets[0] = address(0); // allowing accountSigner permissions for some random contract, consider it as 0 address here + + IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest( + accountSigner, + approvedTargets, + 1 ether, + 0, + type(uint128).max, + 0, + type(uint128).max, + uidCache + ); + + vm.prank(accountAdmin); + bytes memory sig = _signSignerPermissionRequest(permissionsReq); + address account = accountFactory.getAddress(accountAdmin, bytes("")); + IAccountPermissions(payable(account)).setPermissionsForSigner(permissionsReq, sig); + + // As expected, Account Signer is not be able to call setNum on numberContract since it doesnt have numberContract as approved target + assertEq(numberContract.num(), 0); + + vm.prank(accountSigner); + UserOperation[] memory userOp = _setupUserOpExecute( + accountSignerPKey, + bytes(""), + address(numberContract), + 0, + abi.encodeWithSignature("setNum(uint256)", 42) + ); + + vm.expectRevert(); + EntryPoint(entrypoint).handleOps(userOp, beneficiary); + + /*////////////////////////////////////////////////////////// + Attack + //////////////////////////////////////////////////////////////*/ + + //However they can bypass this by using signature verification on number contract instead + vm.prank(accountSigner); + bytes32 digest = keccak256(abi.encode(42)); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(accountSignerPKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert("Account: caller not approved target."); + numberContract.setNumBySignature(account, 42, signature); + assertEq(numberContract.num(), 0); + } +}