Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gateway L2->L1 messaging fixes #725

Merged
merged 5 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridgehub/Bridgehub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
function registerSettlementLayer(
uint256 _newSettlementLayerChainId,
bool _isWhitelisted
) external onlyChainSTM(_newSettlementLayerChainId) onlyL1 {
) external onlyOwner onlyL1 {
whitelistedSettlementLayers[_newSettlementLayerChainId] = _isWhitelisted;
emit SettlementLayerRegistered(_newSettlementLayerChainId, _isWhitelisted);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@
}
}

/// @notice Extracts slice until the end of the array.
/// @dev It is used in one place in order to circumvent the stack too deep error.
function extractSliceUntilEnd(
bytes32[] calldata _proof,
uint256 _start
) internal pure returns (bytes32[] memory slice) {
slice = extractSlice(_proof, _start, _proof.length);
}

/// @inheritdoc IMailbox
function proveL2LeafInclusion(
uint256 _batchNumber,
Expand All @@ -162,8 +171,6 @@
bytes32 _leaf,
bytes32[] calldata _proof
) internal view returns (bool) {
// FIXME: maybe support legacy interface

uint256 ptr = 0;
bytes32 chainIdLeaf;
{
Expand All @@ -177,9 +184,12 @@
);
ptr += logLeafProofLen;

// Note that this logic works only for chains that do not migrate away from the synclayer back to L1.
// Support for chains that migrate back to L1 will be added in the future.
if (s.settlementLayer == address(0)) {
// If the `batchLeafProofLen` is 0, then we assume that this is L1 contract of the top-level
// in the aggregation, i.e. the batch root is stored here on L1.
if (batchLeafProofLen == 0) {
// Double checking that the batch has been executed.
require(_batchNumber <= s.totalBatchesExecuted, "xx");

bytes32 correctBatchRoot = s.l2LogsRootHashes[_batchNumber];
require(correctBatchRoot != bytes32(0), "local root is 0");
return correctBatchRoot == batchSettlementRoot;
Expand All @@ -205,6 +215,7 @@

uint256 settlementLayerBatchNumber;
uint256 settlementLayerBatchRootMask;
address settlementLayerAddress;

// Preventing stack too deep error
{
Expand All @@ -213,14 +224,25 @@
++ptr;
settlementLayerBatchNumber = uint256(settlementLayerPackedBatchInfo >> 128);
settlementLayerBatchRootMask = uint256(settlementLayerPackedBatchInfo & ((1 << 128) - 1));

uint256 settlementLayerChainId = uint256(_proof[ptr]);
++ptr;

// Assuming that `settlementLayerChainId` is an honest chain, the `chainIdLeaf` should belong
// to a chain's message root only if the chain has indeed executed its batch on top of it.
//
// We trust all chains whitelisted by the Bridgehub governance.
require(IBridgehub(s.bridgehub).whitelistedSettlementLayers(settlementLayerChainId), "Mailbox: wrong STM");

settlementLayerAddress = IBridgehub(s.bridgehub).getHyperchain(settlementLayerChainId);
}

return
IMailbox(s.settlementLayer).proveL2LeafInclusion(
IMailbox(settlementLayerAddress).proveL2LeafInclusion(
settlementLayerBatchNumber,
settlementLayerBatchRootMask,
chainIdLeaf,
extractSlice(_proof, ptr, _proof.length)
extractSliceUntilEnd(_proof, ptr)
);
}

Expand All @@ -231,8 +253,6 @@
L2Log memory _log,
bytes32[] calldata _proof
) internal view returns (bool) {
// require(_batchNumber <= s.totalBatchesExecuted, "xx");

bytes32 hashedLog = keccak256(
// solhint-disable-next-line func-named-parameters
abi.encodePacked(_log.l2ShardId, _log.isService, _log.txNumberInBatch, _log.sender, _log.key, _log.value)
Expand Down Expand Up @@ -409,7 +429,7 @@
// Change the sender address if it is a smart contract to prevent address collision between L1 and L2.
// Please note, currently ZKsync address derivation is different from Ethereum one, but it may be changed in the future.
// slither-disable-next-line tx-origin
if (request.sender != tx.origin) {

Check warning on line 432 in l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol

View workflow job for this annotation

GitHub Actions / lint

Avoid to use tx.origin

Check warning on line 432 in l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol

View workflow job for this annotation

GitHub Actions / lint

Avoid to use tx.origin

Check warning on line 432 in l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol

View workflow job for this annotation

GitHub Actions / lint

Avoid to use tx.origin
request.sender = AddressAliasHelper.applyL1ToL2Alias(request.sender);
}

Expand Down
6 changes: 3 additions & 3 deletions l1-contracts/deploy-scripts/Gateway.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

// import {IL1AssetRouter} from "contracts/bridge/interfaces/IL1AssetRouter.sol";

import {IStateTransitionManager} from "contracts/state-transition/IStateTransitionManager.sol";

Check failure on line 24 in l1-contracts/deploy-scripts/Gateway.s.sol

View workflow job for this annotation

GitHub Actions / lint

imported name IStateTransitionManager is not used

Check failure on line 24 in l1-contracts/deploy-scripts/Gateway.s.sol

View workflow job for this annotation

GitHub Actions / lint

imported name IStateTransitionManager is not used

Check failure on line 24 in l1-contracts/deploy-scripts/Gateway.s.sol

View workflow job for this annotation

GitHub Actions / lint

imported name IStateTransitionManager is not used
import {IZkSyncHyperchain} from "contracts/state-transition/chain-interfaces/IZkSyncHyperchain.sol";

contract GatewayScript is Script {
Expand Down Expand Up @@ -117,10 +117,10 @@
}

function registerGateway() public {
IStateTransitionManager stm = IStateTransitionManager(config.stateTransitionProxy);
Ownable ownable = Ownable(config.stateTransitionProxy);
IBridgehub bridgehub = IBridgehub(config.bridgehub);
Ownable ownable = Ownable(config.bridgehub);
vm.prank(ownable.owner());
stm.registerSettlementLayer(config.gatewayChainId, true);
bridgehub.registerSettlementLayer(config.gatewayChainId, true);
// bytes memory data = abi.encodeCall(stm.registerSettlementLayer, (config.chainChainId, true));
// Utils.executeUpgrade({
// _governor: ownable.owner(),
Expand Down
3 changes: 1 addition & 2 deletions l1-contracts/scripts/sync-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,14 +313,13 @@ async function registerSLContractsOnL1(deployer: Deployer) {

console.log(`Gateway chain Id: ${chainId}`);

const l1STM = deployer.stateTransitionManagerContract(deployer.deployWallet);
const l1Bridgehub = deployer.bridgehubContract(deployer.deployWallet);
console.log(deployer.addresses.StateTransition.StateTransitionProxy);
const gatewayAddress = await l1STM.getHyperchain(chainId);
// this script only works when owner is the deployer
console.log("Registering Gateway chain id on the STM");
const receipt1 = await deployer.executeUpgrade(
l1STM.address,
l1Bridgehub.address,
0,
l1Bridgehub.interface.encodeFunctionData("registerSettlementLayer", [chainId, true])
);
Expand Down
6 changes: 3 additions & 3 deletions l1-contracts/src.ts/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1123,9 +1123,9 @@ export class Deployer {
}

public async registerSettlementLayer() {
const stm = this.stateTransitionManagerContract(this.deployWallet);
const calldata = stm.interface.encodeFunctionData("registerSettlementLayer", [this.chainId, true]);
await this.executeUpgrade(this.addresses.StateTransition.StateTransitionProxy, 0, calldata);
const bridgehub = this.bridgehubContract(this.deployWallet);
const calldata = bridgehub.interface.encodeFunctionData("registerSettlementLayer", [this.chainId, true]);
await this.executeUpgrade(this.addresses.Bridgehub.BridgehubProxy, 0, calldata);
if (this.verbose) {
console.log("Gateway registered");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,18 @@ contract MailboxL2LogsProve is MailboxTest {
index = elements.length - 1;
}

// FIXME: restore the test
// function test_RevertWhen_batchNumberGreaterThanBatchesExecuted() public {
// L2Message memory message = L2Message({txNumberInBatch: 0, sender: sender, data: data});
// bytes32[] memory proof = new bytes32[](0);

// vm.expectRevert(bytes("xx"));
// mailboxFacet.proveL2MessageInclusion({
// _batchNumber: batchNumber + 1,
// _index: 0,
// _message: message,
// _proof: proof
// });
// }
function test_RevertWhen_batchNumberGreaterThanBatchesExecuted() public {
L2Message memory message = L2Message({txNumberInBatch: 0, sender: sender, data: data});
bytes32[] memory proof = _appendProofMetadata(new bytes32[](1));

vm.expectRevert(bytes("xx"));
mailboxFacet.proveL2MessageInclusion({
_batchNumber: batchNumber + 1,
_index: 0,
_message: message,
_proof: proof
});
}

function test_success_proveL2MessageInclusion() public {
uint256 firstLogIndex = _addHashedLogToMerkleTree({
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/test/unit_tests/l1_shared_bridge_test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe("Shared Bridge tests", () => {
const revertReason = await getCallRevertReason(
l1SharedBridge.connect(randomSigner).finalizeWithdrawal(chainId, 10, 0, 0, l2ToL1message, dummyProof)
);
expect(revertReason).equal("local root is 0");
expect(revertReason).equal("xx");
});

it("Should revert on finalizing a withdrawal with wrong length of proof", async () => {
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/test/unit_tests/legacy_era_test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe("Legacy Era tests", function () {
const revertReason = await getCallRevertReason(
l1ERC20Bridge.connect(randomSigner).finalizeWithdrawal(10, 0, 0, l2ToL1message, dummyProof)
);
expect(revertReason).equal("local root is 0");
expect(revertReason).equal("xx");
});

it("Should revert on finalizing a withdrawal with wrong proof", async () => {
Expand Down
Loading