feat: 7702 bug fix #1654
feat: 7702 bug fix #1654
Conversation
l1-contracts/test/foundry/l1/unit/concrete/Bridgehub/experimental_bridge.t.sol
Show resolved
Hide resolved
|
|
||
| vm.deal(randomCaller, 1 ether); | ||
| vm.prank(randomCaller); | ||
| vm.expectRevert(abi.encodeWithSelector(MsgValueMismatch.selector, 0, randomCaller.balance)); |
There was a problem hiding this comment.
do we need this first part? It checks that the call would fail with the wrong amount of tokens, which probably should've been covered by other tests
|
|
||
| request.refundRecipient = AddressAliasHelper.actualRefundRecipient(request.refundRecipient, request.sender); | ||
| bool is7702AccountRefundRecipient = EIP_7702_CHECKER.isEIP7702Account(request.refundRecipient); | ||
| bool is7702AccountSender = EIP_7702_CHECKER.isEIP7702Account(request.sender); // This is not the same as refundRecipient, as it appears to be the AR during TwoBridges. |
There was a problem hiding this comment.
This is not the same as refundRecipient, as it appears to be the AR during TwoBridges.
do we need this then?
There was a problem hiding this comment.
In case it's a direct tx EOA will be the sender and for 7702 we don't want it to be aliased
| // If the `_refundRecipient` is not provided, we use the `_originalCaller` as the recipient. | ||
| // solhint-disable avoid-tx-origin | ||
| // slither-disable-next-line tx-origin | ||
| _recipient = (_originalCaller == tx.origin || _is7702Account) |
There was a problem hiding this comment.
why do we have _is7702Account here? Isn't it related to _refundRecipient, and not _originalCaller?
There was a problem hiding this comment.
maybe it has to be removed
l1-contracts/package.json
Outdated
| "build": "hardhat compile && CONTRACTS_BASE_NETWORK_ZKSYNC=true hardhat compile ", | ||
| "build-l1": "hardhat compile", | ||
| "build:foundry": "forge build && forge build --zksync --skip '*/l1-contracts/test/*'", | ||
| "build:foundry": "forge build && forge build --zksync --skip '*/l1-contracts/test/*' --skip 'deploy-scripts/*'", |
| // TODO: remove comment to check that refund recipient is correct after foundry version is bumped to post-prague | ||
| // assertEq(address(uint160(request.transaction.reserved[1])), randomCaller, "Refund recipient mismatch"); |
There was a problem hiding this comment.
Please read this thread.
I think you can bump the foundry? Or were there other issues with that as well, apart from just choosing a foundry version?
| DeployedContracts memory contracts; | ||
|
|
||
| contracts.multicall3 = address(new Multicall3{salt: salt}()); | ||
| contracts.eip7702Checker = address(new MockEIP7702Checker{salt: salt}()); |
There was a problem hiding this comment.
Here we deploy MockEIP7702Checker. Should we use EIP7702Checker?
Co-authored-by: Raid Ateir <[email protected]> Co-authored-by: 0xValera <[email protected]>
| /// @title EIP7702Checker | ||
| /// @notice Utility to detect EIP-7702 style EOAs (accounts with code stubs) | ||
| /// @dev See: https://eips.ethereum.org/EIPS/eip-7702 | ||
|
|
There was a problem hiding this comment.
nit: unneeded newline
| /** | ||
| * @notice Returns true if the given account has exactly the EIP-7702 code stub. | ||
| * @param account The address to check. | ||
| * @return isStub True if the account matches the EIP-7702 code pattern. |
There was a problem hiding this comment.
why is it called stub?
| /// @title EIP7702Checker | ||
| /// @notice Utility to detect EIP-7702 style EOAs (accounts with code stubs) | ||
| /// @dev See: https://eips.ethereum.org/EIPS/eip-7702 | ||
|
|
There was a problem hiding this comment.
nit: empty newline
| /// @notice Address of the Multicall3 contract. | ||
| address multicall3; | ||
| /// @notice Address of the EIP7702Checker contract. | ||
| address eip7702Checker; |
There was a problem hiding this comment.
maybe makes sense to remove it fully from this file?
| // If the `_refundRecipient` is not provided, we use the `_originalCaller` as the recipient. | ||
| // solhint-disable avoid-tx-origin | ||
| // slither-disable-next-line tx-origin | ||
| _recipient = (_originalCaller == tx.origin) |
There was a problem hiding this comment.
what if the original caller is an eip712 account? then it is possible that _originalCaller != tx.origin, but we still should not apply aliasing
|
|
||
| function getCreationCodeEVM(string memory contractIdentifier) internal view returns (bytes memory) { | ||
| string[3] memory DA_CONTRACT_IDENTIFIERS = ["RollupL1DAValidator", "AvailL1DAValidator", "DummyAvailBridge"]; | ||
| string[4] memory DA_CONTRACT_IDENTIFIERS = [ |
There was a problem hiding this comment.
maybe makes sense to update the variable name? I understand that we may not want to update the folder name in this PR, but still wherever possible, let's keep the variable names aligned with code...
| return abi.encode(config.l1ChainId, addresses.daAddresses.rollupDAManager); | ||
| } else if (compareStrings(contractName, "MailboxFacet")) { | ||
| return abi.encode(config.eraChainId, config.l1ChainId); | ||
| return abi.encode(config.eraChainId, config.l1ChainId, addresses.daAddresses.eip7702Checker); |
There was a problem hiding this comment.
even though the checker is inside da-contracts folder, the only reason it is there is because it is a dedicated folder for l1-only contracts, let's not call it a da contract inside the script
4 participants
What ❔
Why ❔
Checklist