Skip to content

Commit 2108627

Browse files
nkrishangnkrishang
authored
Fix vuln: eip-1271 caller must be approved target (#474)
1 parent c5903b2 commit 2108627

File tree

3 files changed

+299
-2
lines changed

3 files changed

+299
-2
lines changed

‎contracts/smart-wallet/non-upgradeable/Account.sol‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,15 @@ contract Account is
165165
{
166166
address signer = _hash.recover(_signature);
167167

168-
if (isAdmin(signer) || isActiveSigner(signer)) {
168+
if (isAdmin(signer)) {
169+
return MAGICVALUE;
170+
}
171+
172+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
173+
address caller = msg.sender;
174+
require(data.approvedTargets[signer].contains(caller), "Account: caller not approved target.");
175+
176+
if (isActiveSigner(signer)) {
169177
magicValue = MAGICVALUE;
170178
}
171179
}

‎contracts/smart-wallet/utils/AccountCore.sol‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,15 @@ contract AccountCore is IAccountCore, Initializable, Multicall, BaseAccount, ERC
143143
{
144144
address signer = _hash.recover(_signature);
145145

146-
if (isAdmin(signer) || isActiveSigner(signer)) {
146+
if (isAdmin(signer)) {
147+
return MAGICVALUE;
148+
}
149+
150+
AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
151+
address caller = msg.sender;
152+
require(data.approvedTargets[signer].contains(caller), "Account: caller not approved target.");
153+
154+
if (isActiveSigner(signer)) {
147155
magicValue = MAGICVALUE;
148156
}
149157
}

‎src/test/smart-wallet/AccountVulnPOC.t.sol‎

Lines changed: 281 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,281 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
pragma solidity ^0.8.0;
3+
4+
// Test utils
5+
import "../utils/BaseTest.sol";
6+
// Account Abstraction setup for smart wallets.
7+
import { EntryPoint, IEntryPoint } from "contracts/smart-wallet/utils/Entrypoint.sol";
8+
import { UserOperation } from "contracts/smart-wallet/utils/UserOperation.sol";
9+
10+
// Target
11+
import { IAccountPermissions } from "contracts/extension/interface/IAccountPermissions.sol";
12+
import { AccountFactory, Account } from "contracts/smart-wallet/non-upgradeable/AccountFactory.sol";
13+
14+
library GPv2EIP1271 {
15+
bytes4 internal constant MAGICVALUE = 0x1626ba7e;
16+
}
17+
18+
interface EIP1271Verifier {
19+
function isValidSignature(bytes32 _hash, bytes memory _signature) external view returns (bytes4 magicValue);
20+
}
21+
22+
/// @dev This is a dummy contract to test contract interactions with Account.
23+
contract Number {
24+
uint256 public num;
25+
26+
function setNum(uint256 _num) public {
27+
num = _num;
28+
}
29+
30+
function doubleNum() public {
31+
num *= 2;
32+
}
33+
34+
function incrementNum() public {
35+
num += 1;
36+
}
37+
38+
function setNumBySignature(
39+
address owner,
40+
uint256 newNum,
41+
bytes calldata signature
42+
) public {
43+
if (owner.code.length == 0) {
44+
// Signature verification by ECDSA
45+
} else {
46+
// Signature verfication by EIP1271
47+
bytes32 digest = keccak256(abi.encode(newNum));
48+
require(
49+
EIP1271Verifier(owner).isValidSignature(digest, signature) == GPv2EIP1271.MAGICVALUE,
50+
"GPv2: invalid eip1271 signature"
51+
);
52+
num = newNum;
53+
}
54+
}
55+
}
56+
57+
contract SimpleAccountVulnPOCTest is BaseTest {
58+
// Target contracts
59+
EntryPoint private entrypoint;
60+
AccountFactory private accountFactory;
61+
62+
// Mocks
63+
Number internal numberContract;
64+
65+
// Test params
66+
uint256 private accountAdminPKey = 100;
67+
address private accountAdmin;
68+
69+
uint256 private accountSignerPKey = 200;
70+
address private accountSigner;
71+
72+
uint256 private nonSignerPKey = 300;
73+
address private nonSigner;
74+
75+
// UserOp terminology: `sender` is the smart wallet.
76+
address private sender = 0xBB956D56140CA3f3060986586A2631922a4B347E;
77+
address payable private beneficiary = payable(address(0x45654));
78+
79+
bytes32 private uidCache = bytes32("random uid");
80+
81+
event AccountCreated(address indexed account, address indexed accountAdmin);
82+
83+
function _signSignerPermissionRequest(IAccountPermissions.SignerPermissionRequest memory _req)
84+
internal
85+
view
86+
returns (bytes memory signature)
87+
{
88+
bytes32 typehashSignerPermissionRequest = keccak256(
89+
"SignerPermissionRequest(address signer,address[] approvedTargets,uint256 nativeTokenLimitPerTransaction,uint128 permissionStartTimestamp,uint128 permissionEndTimestamp,uint128 reqValidityStartTimestamp,uint128 reqValidityEndTimestamp,bytes32 uid)"
90+
);
91+
bytes32 nameHash = keccak256(bytes("Account"));
92+
bytes32 versionHash = keccak256(bytes("1"));
93+
bytes32 typehashEip712 = keccak256(
94+
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
95+
);
96+
bytes32 domainSeparator = keccak256(abi.encode(typehashEip712, nameHash, versionHash, block.chainid, sender));
97+
98+
bytes memory encodedRequest = abi.encode(
99+
typehashSignerPermissionRequest,
100+
_req.signer,
101+
keccak256(abi.encodePacked(_req.approvedTargets)),
102+
_req.nativeTokenLimitPerTransaction,
103+
_req.permissionStartTimestamp,
104+
_req.permissionEndTimestamp,
105+
_req.reqValidityStartTimestamp,
106+
_req.reqValidityEndTimestamp,
107+
_req.uid
108+
);
109+
bytes32 structHash = keccak256(encodedRequest);
110+
bytes32 typedDataHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
111+
112+
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountAdminPKey, typedDataHash);
113+
signature = abi.encodePacked(r, s, v);
114+
}
115+
116+
function _setupUserOp(
117+
uint256 _signerPKey,
118+
bytes memory _initCode,
119+
bytes memory _callDataForEntrypoint
120+
) internal returns (UserOperation[] memory ops) {
121+
uint256 nonce = entrypoint.getNonce(sender, 0);
122+
123+
// Get user op fields
124+
UserOperation memory op = UserOperation({
125+
sender: sender,
126+
nonce: nonce,
127+
initCode: _initCode,
128+
callData: _callDataForEntrypoint,
129+
callGasLimit: 500_000,
130+
verificationGasLimit: 500_000,
131+
preVerificationGas: 500_000,
132+
maxFeePerGas: 0,
133+
maxPriorityFeePerGas: 0,
134+
paymasterAndData: bytes(""),
135+
signature: bytes("")
136+
});
137+
138+
// Sign UserOp
139+
bytes32 opHash = EntryPoint(entrypoint).getUserOpHash(op);
140+
bytes32 msgHash = ECDSA.toEthSignedMessageHash(opHash);
141+
142+
(uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPKey, msgHash);
143+
bytes memory userOpSignature = abi.encodePacked(r, s, v);
144+
145+
address recoveredSigner = ECDSA.recover(msgHash, v, r, s);
146+
address expectedSigner = vm.addr(_signerPKey);
147+
assertEq(recoveredSigner, expectedSigner);
148+
149+
op.signature = userOpSignature;
150+
151+
// Store UserOp
152+
ops = new UserOperation[](1);
153+
ops[0] = op;
154+
}
155+
156+
function _setupUserOpExecute(
157+
uint256 _signerPKey,
158+
bytes memory _initCode,
159+
address _target,
160+
uint256 _value,
161+
bytes memory _callData
162+
) internal returns (UserOperation[] memory) {
163+
bytes memory callDataForEntrypoint = abi.encodeWithSignature(
164+
"execute(address,uint256,bytes)",
165+
_target,
166+
_value,
167+
_callData
168+
);
169+
170+
return _setupUserOp(_signerPKey, _initCode, callDataForEntrypoint);
171+
}
172+
173+
function _setupUserOpExecuteBatch(
174+
uint256 _signerPKey,
175+
bytes memory _initCode,
176+
address[] memory _target,
177+
uint256[] memory _value,
178+
bytes[] memory _callData
179+
) internal returns (UserOperation[] memory) {
180+
bytes memory callDataForEntrypoint = abi.encodeWithSignature(
181+
"executeBatch(address[],uint256[],bytes[])",
182+
_target,
183+
_value,
184+
_callData
185+
);
186+
187+
return _setupUserOp(_signerPKey, _initCode, callDataForEntrypoint);
188+
}
189+
190+
function setUp() public override {
191+
super.setUp();
192+
193+
// Setup signers.
194+
accountAdmin = vm.addr(accountAdminPKey);
195+
vm.deal(accountAdmin, 100 ether);
196+
197+
accountSigner = vm.addr(accountSignerPKey);
198+
nonSigner = vm.addr(nonSignerPKey);
199+
200+
// Setup contracts
201+
entrypoint = new EntryPoint();
202+
// deploy account factory
203+
accountFactory = new AccountFactory(IEntryPoint(payable(address(entrypoint))));
204+
// deploy dummy contract
205+
numberContract = new Number();
206+
}
207+
208+
/*//////////////////////////////////////////////////////////
209+
Test: performing a contract call
210+
//////////////////////////////////////////////////////////////*/
211+
212+
function _setup_executeTransaction() internal {
213+
bytes memory initCallData = abi.encodeWithSignature("createAccount(address,bytes)", accountAdmin, bytes(""));
214+
bytes memory initCode = abi.encodePacked(abi.encodePacked(address(accountFactory)), initCallData);
215+
216+
UserOperation[] memory userOpCreateAccount = _setupUserOpExecute(
217+
accountAdminPKey,
218+
initCode,
219+
address(0),
220+
0,
221+
bytes("")
222+
);
223+
224+
EntryPoint(entrypoint).handleOps(userOpCreateAccount, beneficiary);
225+
}
226+
227+
function test_POC() public {
228+
_setup_executeTransaction();
229+
230+
/*//////////////////////////////////////////////////////////
231+
Setup
232+
//////////////////////////////////////////////////////////////*/
233+
address[] memory approvedTargets = new address[](1);
234+
approvedTargets[0] = address(0); // allowing accountSigner permissions for some random contract, consider it as 0 address here
235+
236+
IAccountPermissions.SignerPermissionRequest memory permissionsReq = IAccountPermissions.SignerPermissionRequest(
237+
accountSigner,
238+
approvedTargets,
239+
1 ether,
240+
0,
241+
type(uint128).max,
242+
0,
243+
type(uint128).max,
244+
uidCache
245+
);
246+
247+
vm.prank(accountAdmin);
248+
bytes memory sig = _signSignerPermissionRequest(permissionsReq);
249+
address account = accountFactory.getAddress(accountAdmin, bytes(""));
250+
IAccountPermissions(payable(account)).setPermissionsForSigner(permissionsReq, sig);
251+
252+
// As expected, Account Signer is not be able to call setNum on numberContract since it doesnt have numberContract as approved target
253+
assertEq(numberContract.num(), 0);
254+
255+
vm.prank(accountSigner);
256+
UserOperation[] memory userOp = _setupUserOpExecute(
257+
accountSignerPKey,
258+
bytes(""),
259+
address(numberContract),
260+
0,
261+
abi.encodeWithSignature("setNum(uint256)", 42)
262+
);
263+
264+
vm.expectRevert();
265+
EntryPoint(entrypoint).handleOps(userOp, beneficiary);
266+
267+
/*//////////////////////////////////////////////////////////
268+
Attack
269+
//////////////////////////////////////////////////////////////*/
270+
271+
//However they can bypass this by using signature verification on number contract instead
272+
vm.prank(accountSigner);
273+
bytes32 digest = keccak256(abi.encode(42));
274+
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountSignerPKey, digest);
275+
bytes memory signature = abi.encodePacked(r, s, v);
276+
277+
vm.expectRevert("Account: caller not approved target.");
278+
numberContract.setNumBySignature(account, 42, signature);
279+
assertEq(numberContract.num(), 0);
280+
}
281+
}

0 commit comments

Comments
 (0)