Skip to content

Commit c0edec2

Browse files
ilikesymmetryilikesymmetry
committed
Add review comments
1 parent 2523cb7 commit c0edec2

File tree

4 files changed

+74
-94
lines changed

4 files changed

+74
-94
lines changed

‎src/EIP7702Proxy.sol‎

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,88 +1,90 @@
11
// SPDX-License-Identifier: UNLICENSED
22
pragma solidity ^0.8.0;
33

4+
/// @review does Solady v0.1 have something equivalent offerings for these depedencies? would be nice to use what we recently paid to audit and would probably also be optimized
45
import {Proxy} from "openzeppelin-contracts/contracts/proxy/Proxy.sol";
56
import {ERC1967Utils} from "openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Utils.sol";
67
import {ECDSA} from "openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol";
78
import {Address} from "openzeppelin-contracts/contracts/utils/Address.sol";
89

910
/// @title EIP7702Proxy
10-
/// @notice Proxy contract designed for EIP-7702 smart accounts
11-
/// @dev Implements ERC-1967 with an initial implementation and guarded initialization
11+
/// @notice Proxy contract designed for EIP-7702 smart accounts.
12+
/// @dev Implements ERC-1967 with an initial implementation and guarded initialization.
13+
/// @author Coinbase (https://github.com/base-org/eip-7702-proxy).
1214
contract EIP7702Proxy is Proxy {
13-
// ERC1271 interface constants
15+
/// @notice ERC1271 interface constants
1416
bytes4 internal constant ERC1271_MAGIC_VALUE = 0x1626ba7e;
15-
bytes4 internal constant ERC1271_ISVALIDSIGNATURE_SELECTOR = 0x1626ba7e;
1617
bytes4 internal constant ERC1271_FAIL_VALUE = 0xffffffff;
1718

1819
/// @notice Address of this proxy contract (stored as immutable)
1920
address immutable proxy;
21+
2022
/// @notice Initial implementation address set during construction
2123
address immutable initialImplementation;
24+
2225
/// @notice Function selector on the implementation that is guarded from direct calls
2326
bytes4 immutable guardedInitializer;
2427

28+
/// @review natspec
2529
event Upgraded(address indexed implementation);
2630

31+
/// @review natspec
2732
error InvalidSignature();
33+
34+
/// @review natspec
2835
error InvalidInitializer();
36+
37+
/// @review natspec
2938
error InvalidImplementation();
3039

40+
/// @review natspec
3141
constructor(address implementation, bytes4 initializer) {
3242
proxy = address(this);
3343
initialImplementation = implementation;
44+
/// @review what happens if `initializer` is one of the selectors we define in this contract?
3445
guardedInitializer = initializer;
3546
}
3647

3748
/// @notice Initializes the proxy and implementation with a signed payload
49+
///
3850
/// @param args The initialization arguments for the implementation
3951
/// @param signature The signature authorizing initialization
52+
///
4053
/// @dev Signature must be from this contract's address
41-
function initialize(
42-
bytes calldata args,
43-
bytes calldata signature
44-
) external {
54+
function initialize(bytes calldata args, bytes calldata signature) external {
4555
// construct hash incompatible with wallet RPCs to avoid phishing
4656
bytes32 hash = keccak256(abi.encode(proxy, args));
4757
address recovered = ECDSA.recover(hash, signature);
4858
if (recovered != address(this)) revert InvalidSignature();
4959

5060
// enforce initialization only on initial implementation
5161
address implementation = _implementation();
52-
if (implementation != initialImplementation)
53-
revert InvalidImplementation();
62+
if (implementation != initialImplementation) revert InvalidImplementation();
5463

5564
// Set the ERC-1967 implementation slot and emit Upgraded event
5665
ERC1967Utils.upgradeToAndCall(initialImplementation, "");
57-
58-
Address.functionDelegateCall(
59-
initialImplementation,
60-
abi.encodePacked(guardedInitializer, args)
61-
);
66+
/// @review we can just put the initializer calldata into the second arg of `upgradeToAndCall` above correct?
67+
Address.functionDelegateCall(initialImplementation, abi.encodePacked(guardedInitializer, args));
6268
}
6369

70+
/// @review let's use consistent comment format `///`, wdyt?
6471
/**
6572
* @notice Handles ERC-1271 signature validation by enforcing a final ecrecover check if signatures fail `isValidSignature` check
73+
*
6674
* @dev This ensures EOA signatures are considered valid regardless of the implementation's `isValidSignature` implementation
75+
*
6776
* @param hash The hash of the message being signed
6877
* @param signature The signature of the message
78+
*
6979
* @return The result of the `isValidSignature` check
7080
*/
71-
function isValidSignature(
72-
bytes32 hash,
73-
bytes calldata signature
74-
) external returns (bytes4) {
81+
function isValidSignature(bytes32 hash, bytes calldata signature) external returns (bytes4) {
7582
// First try delegatecall to implementation
76-
(bool success, bytes memory result) = _implementation().delegatecall(
77-
msg.data
78-
);
83+
(bool success, bytes memory result) = _implementation().delegatecall(msg.data);
7984

8085
// If delegatecall succeeded and returned magic value, return that
81-
if (
82-
success &&
83-
result.length == 32 &&
84-
abi.decode(result, (bytes4)) == ERC1271_MAGIC_VALUE
85-
) {
86+
if (success && result.length == 32 && abi.decode(result, (bytes4)) == ERC1271_MAGIC_VALUE) {
87+
/// @review this selective use of assembly reads kind of funny lol, how much gas are we saving here?
8688
assembly {
8789
mstore(0, ERC1271_MAGIC_VALUE)
8890
return(0, 32)
@@ -107,13 +109,11 @@ contract EIP7702Proxy is Proxy {
107109
}
108110
}
109111

112+
/// @review Do we need to override this now that we are setting the implementation at initialization?
110113
/// @inheritdoc Proxy
111114
function _implementation() internal view override returns (address) {
112115
address implementation = ERC1967Utils.getImplementation();
113-
return
114-
implementation != address(0)
115-
? implementation
116-
: initialImplementation;
116+
return implementation != address(0) ? implementation : initialImplementation;
117117
}
118118

119119
/// @inheritdoc Proxy
@@ -123,6 +123,7 @@ contract EIP7702Proxy is Proxy {
123123
// block guarded initializer from being called
124124
if (msg.sig == guardedInitializer) revert InvalidInitializer();
125125

126+
/// @review should we prevent all delegate calls if the implementation is not yet initialized? Feels like a good default protection
126127
_delegate(_implementation());
127128
}
128129
}

‎test/EIP7702Proxy/delegate.t.sol‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,39 +5,42 @@ import {EIP7702ProxyBase} from "../base/EIP7702ProxyBase.sol";
55
import {EIP7702Proxy} from "../../src/EIP7702Proxy.sol";
66
import {CoinbaseSmartWallet} from "../../lib/smart-wallet/src/CoinbaseSmartWallet.sol";
77

8+
/// @review In general I think we should split our tests here between implementation-agnostic invariants the proxy maintains and separate integration tests specific to CoinbaseSmartWallet
89
contract DelegateTest is EIP7702ProxyBase {
910
function setUp() public override {
1011
super.setUp();
11-
12+
1213
// Initialize the proxy for delegation tests
1314
bytes memory initArgs = _createInitArgs(_newOwner);
1415
bytes memory signature = _signInitData(_EOA_PRIVATE_KEY, initArgs);
1516
vm.prank(_eoa);
1617
EIP7702Proxy(_eoa).initialize(initArgs, signature);
1718
}
1819

20+
/// @review prefer fuzzing where possible
1921
function testBlocksGuardedInitializer() public {
20-
bytes memory initData = abi.encodeWithSelector(
21-
CoinbaseSmartWallet.initialize.selector,
22-
_createInitArgs(_newOwner)
23-
);
22+
bytes memory initData =
23+
abi.encodeWithSelector(CoinbaseSmartWallet.initialize.selector, _createInitArgs(_newOwner));
2424

2525
vm.expectRevert(EIP7702Proxy.InvalidInitializer.selector);
2626
address(_eoa).call(initData);
2727
}
2828

29+
/// @review add test for non-binary return data, e.g. ownerAtIndex
30+
/// @review add test for if read fails, then delegate fails
31+
/// @review add test for if implementation slot is empty
2932
function testDelegatesReadCall() public {
30-
assertTrue(
31-
CoinbaseSmartWallet(payable(_eoa)).isOwnerAddress(_newOwner),
32-
"Delegated read call should succeed"
33-
);
33+
assertTrue(CoinbaseSmartWallet(payable(_eoa)).isOwnerAddress(_newOwner), "Delegated read call should succeed");
3434
}
3535

36+
/// @review prefer fuzzing where possible
37+
/// @review add test for if write fails, then delegate fails
38+
/// @review add test for if implementation slot is empty
3639
function testDelegatesWriteCall() public {
3740
// Test a state-changing call
3841
address recipient = address(0xBEEF);
3942
uint256 amount = 1 ether;
40-
43+
4144
// Fund the proxy
4245
vm.deal(address(_eoa), amount);
4346

@@ -48,10 +51,6 @@ contract DelegateTest is EIP7702ProxyBase {
4851
"" // empty calldata for simple transfer
4952
);
5053

51-
assertEq(
52-
recipient.balance,
53-
amount,
54-
"Delegated write call should transfer ETH"
55-
);
54+
assertEq(recipient.balance, amount, "Delegated write call should transfer ETH");
5655
}
57-
}
56+
}

‎test/EIP7702Proxy/initialize.t.sol‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import {CoinbaseSmartWallet} from "../../lib/smart-wallet/src/CoinbaseSmartWalle
77
import {ECDSA} from "openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol";
88
import {ERC1967Utils} from "openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Utils.sol";
99

10+
/// @review In general I think we should split our tests here between implementation-agnostic invariants the proxy maintains and separate integration tests specific to CoinbaseSmartWallet
1011
contract InitializeTest is EIP7702ProxyBase {
12+
/// @review style should be snake_case, e.g. test_success_validSignature
13+
/// @review prefer fuzzing where possible
14+
/// @review add test where fails because initializer delegatecall fails
1115
function testSucceedsWithValidSignature() public {
1216
bytes memory initArgs = _createInitArgs(_newOwner);
1317
bytes memory signature = _signInitData(_EOA_PRIVATE_KEY, initArgs);
@@ -16,12 +20,10 @@ contract InitializeTest is EIP7702ProxyBase {
1620

1721
// Verify initialization through implementation at the EOA's address
1822
CoinbaseSmartWallet wallet = CoinbaseSmartWallet(payable(_eoa));
19-
assertTrue(
20-
wallet.isOwnerAddress(_newOwner),
21-
"New owner should be owner after initialization"
22-
);
23+
assertTrue(wallet.isOwnerAddress(_newOwner), "New owner should be owner after initialization");
2324
}
2425

26+
/// @review style should be snake_case
2527
function testRevertsWithInvalidSignature() public {
2628
bytes memory initArgs = _createInitArgs(_newOwner);
2729
bytes memory signature = hex"deadbeef"; // Invalid signature
@@ -30,6 +32,8 @@ contract InitializeTest is EIP7702ProxyBase {
3032
EIP7702Proxy(_eoa).initialize(initArgs, signature);
3133
}
3234

35+
/// @review prefer fuzzing where possible
36+
/// @review add more tests like this for replay protection cases (same sig, different proxy or args)
3337
function testRevertsWithWrongSigner() public {
3438
// Create signature with different private key
3539
uint256 wrongPk = 0xC0FFEE; // Using a different key than either EOA or new owner
@@ -43,6 +47,8 @@ contract InitializeTest is EIP7702ProxyBase {
4347
EIP7702Proxy(_eoa).initialize(initArgs, signature);
4448
}
4549

50+
/// @review prefer fuzzing where possible
51+
/// @review this is specific to the implementation, would prefer to segment into an integration test suite with CoinbaseSmartWallet
4652
function testCanOnlyBeCalledOnce() public {
4753
bytes memory initArgs = _createInitArgs(_newOwner);
4854
bytes memory signature = _signInitData(_EOA_PRIVATE_KEY, initArgs);
@@ -54,6 +60,10 @@ contract InitializeTest is EIP7702ProxyBase {
5460
EIP7702Proxy(_eoa).initialize(initArgs, signature);
5561
}
5662

63+
/// @review prefer fuzzing where possible
64+
/// @review think we should have a test that mocks users having the 1967 slot already set to another implementation.
65+
/// For example they delegate to something metamask sets up and then move over the CBSW. How does our contract behave in these scenarios?
66+
/// @review add test for if implementation slot has been changed to non-original value
5767
function testSetsERC1967ImplementationSlot() public {
5868
bytes memory initArgs = _createInitArgs(_newOwner);
5969
bytes memory signature = _signInitData(_EOA_PRIVATE_KEY, initArgs);
@@ -62,9 +72,7 @@ contract InitializeTest is EIP7702ProxyBase {
6272

6373
address storedImpl = _getERC1967Implementation(address(_eoa));
6474
assertEq(
65-
storedImpl,
66-
address(_implementation),
67-
"ERC1967 implementation slot should store implementation address"
75+
storedImpl, address(_implementation), "ERC1967 implementation slot should store implementation address"
6876
);
6977
}
7078

‎test/EIP7702Proxy/isValidSignature.t.sol‎

Lines changed: 12 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {EIP7702Proxy} from "../../src/EIP7702Proxy.sol";
66
import {CoinbaseSmartWallet} from "../../lib/smart-wallet/src/CoinbaseSmartWallet.sol";
77
import {ECDSA} from "openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol";
88

9+
/// @review In general I think we should split our tests here between implementation-agnostic invariants the proxy maintains and separate integration tests specific to CoinbaseSmartWallet
910
contract IsValidSignatureTest is EIP7702ProxyBase {
1011
bytes4 constant ERC1271_MAGIC_VALUE = 0x1626ba7e;
1112
bytes4 constant ERC1271_FAIL_VALUE = 0xffffffff;
@@ -26,17 +27,13 @@ contract IsValidSignatureTest is EIP7702ProxyBase {
2627
testHash = keccak256("test message");
2728

2829
// Verify owner was set correctly
29-
assertTrue(
30-
wallet.isOwnerAddress(_newOwner),
31-
"New owner should be set after initialization"
32-
);
33-
assertEq(
34-
wallet.ownerAtIndex(0),
35-
abi.encode(_newOwner),
36-
"Owner at index 0 should be new owner"
37-
);
30+
assertTrue(wallet.isOwnerAddress(_newOwner), "New owner should be set after initialization");
31+
assertEq(wallet.ownerAtIndex(0), abi.encode(_newOwner), "Owner at index 0 should be new owner");
3832
}
3933

34+
/// @review add test for calling on uninitialized contract
35+
/// @review add test if delegatecall reverts then bubbles up revert data
36+
4037
function testValidContractOwnerSignature() public {
4138
// Create signature from contract owner
4239
bytes memory signature = _createOwnerSignature(
@@ -47,11 +44,7 @@ contract IsValidSignatureTest is EIP7702ProxyBase {
4744
);
4845

4946
bytes4 result = wallet.isValidSignature(testHash, signature);
50-
assertEq(
51-
result,
52-
ERC1271_MAGIC_VALUE,
53-
"Should accept valid contract owner signature"
54-
);
47+
assertEq(result, ERC1271_MAGIC_VALUE, "Should accept valid contract owner signature");
5548
}
5649

5750
function testValidEOASignature() public {
@@ -60,11 +53,7 @@ contract IsValidSignatureTest is EIP7702ProxyBase {
6053
bytes memory signature = abi.encodePacked(r, s, v);
6154

6255
bytes4 result = wallet.isValidSignature(testHash, signature);
63-
assertEq(
64-
result,
65-
ERC1271_MAGIC_VALUE,
66-
"Should accept valid EOA signature"
67-
);
56+
assertEq(result, ERC1271_MAGIC_VALUE, "Should accept valid EOA signature");
6857
}
6958

7059
function testInvalidEOASignature() public {
@@ -74,39 +63,22 @@ contract IsValidSignatureTest is EIP7702ProxyBase {
7463
bytes memory signature = abi.encodePacked(r, s, v);
7564

7665
bytes4 result = wallet.isValidSignature(testHash, signature);
77-
assertEq(
78-
result,
79-
ERC1271_FAIL_VALUE,
80-
"Should reject invalid EOA signature"
81-
);
66+
assertEq(result, ERC1271_FAIL_VALUE, "Should reject invalid EOA signature");
8267
}
8368

8469
function testInvalidOwnerSignature() public {
8570
// Create a valid format signature but with wrong signer
8671
uint256 wrongPk = 0xBADBAD; // Different from both EOA and new owner (0xB0B)
87-
bytes memory signature = _createOwnerSignature(
88-
testHash,
89-
address(wallet),
90-
wrongPk,
91-
0
92-
);
72+
bytes memory signature = _createOwnerSignature(testHash, address(wallet), wrongPk, 0);
9373

9474
bytes4 result = wallet.isValidSignature(testHash, signature);
95-
assertEq(
96-
result,
97-
ERC1271_FAIL_VALUE,
98-
"Should reject signature from non-owner"
99-
);
75+
assertEq(result, ERC1271_FAIL_VALUE, "Should reject signature from non-owner");
10076
}
10177

10278
function testEmptySignature() public {
10379
bytes memory emptySignature = "";
10480

10581
bytes4 result = wallet.isValidSignature(testHash, emptySignature);
106-
assertEq(
107-
result,
108-
ERC1271_FAIL_VALUE,
109-
"Should reject empty signature"
110-
);
82+
assertEq(result, ERC1271_FAIL_VALUE, "Should reject empty signature");
11183
}
11284
}

0 commit comments

Comments
 (0)