From 0dc632dafe2489064607df715a00a38e9858050d Mon Sep 17 00:00:00 2001 From: Ignasi Date: Tue, 17 Sep 2024 14:40:21 +0200 Subject: [PATCH] Review fixes --- .../IBasePolygonZkEVMGlobalExitRoot.sol | 2 +- .../interfaces/IBridgeL2SovereignChains.sol | 5 +++ .../BridgeL2SovereignChain.sol | 23 +++++++---- .../GlobalExitRootManagerL2SovereignChain.sol | 28 ++----------- .../BridgeL2GasTokensSovereignChains.test.ts | 39 +++++-------------- .../BridgeL2SovereignChain.test.ts | 1 - 6 files changed, 34 insertions(+), 64 deletions(-) diff --git a/contracts/interfaces/IBasePolygonZkEVMGlobalExitRoot.sol b/contracts/interfaces/IBasePolygonZkEVMGlobalExitRoot.sol index f1b0a1b1..414df355 100644 --- a/contracts/interfaces/IBasePolygonZkEVMGlobalExitRoot.sol +++ b/contracts/interfaces/IBasePolygonZkEVMGlobalExitRoot.sol @@ -9,7 +9,7 @@ interface IBasePolygonZkEVMGlobalExitRoot { error OnlyAllowedContracts(); /** - * @dev Thrown when the caller is not the trusted sequencer + * @dev Thrown when the caller is not the coinbase */ error OnlyCoinbase(); diff --git a/contracts/v2/interfaces/IBridgeL2SovereignChains.sol b/contracts/v2/interfaces/IBridgeL2SovereignChains.sol index 83af2f72..eb99460a 100644 --- a/contracts/v2/interfaces/IBridgeL2SovereignChains.sol +++ b/contracts/v2/interfaces/IBridgeL2SovereignChains.sol @@ -26,6 +26,11 @@ interface IBridgeL2SovereignChains is IPolygonZkEVMBridgeV2 { */ error NotValidBridgeManager(); + /** + * @dev Thrown when trying to remove a token mapping that has not been mapped before + */ + error TokenNotMapped(); + function initialize( uint32 _networkID, address _gasTokenAddress, diff --git a/contracts/v2/sovereignChains/BridgeL2SovereignChain.sol b/contracts/v2/sovereignChains/BridgeL2SovereignChain.sol index e67d6738..731ab36d 100644 --- a/contracts/v2/sovereignChains/BridgeL2SovereignChain.sol +++ b/contracts/v2/sovereignChains/BridgeL2SovereignChain.sol @@ -2,15 +2,10 @@ pragma solidity 0.8.20; -import "../lib/DepositContractV2.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; -import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/IERC20MetadataUpgradeable.sol"; import "../../lib/TokenWrapped.sol"; import "../../interfaces/IBasePolygonZkEVMGlobalExitRoot.sol"; -import "../../interfaces/IBridgeMessageReceiver.sol"; import "../interfaces/IBridgeL2SovereignChains.sol"; -import "../../lib/EmergencyManager.sol"; -import "../../lib/GlobalExitRootLib.sol"; import "../PolygonZkEVMBridgeV2.sol"; /** @@ -21,6 +16,9 @@ contract BridgeL2SovereignChain is PolygonZkEVMBridgeV2, IBridgeL2SovereignChains { + + using SafeERC20Upgradeable for IERC20Upgradeable; + // Map to store wrappedAddresses that are not mintable mapping(address wrappedAddress => bool isNotMintable) public wrappedAddressIsNotMintable; @@ -92,7 +90,7 @@ contract BridgeL2SovereignChain is // or (2) wrapped with custom contract from origin network if (wrappedAddressIsNotMintable[address(tokenWrapped)]) { // Don't use burn but transfer to bridge - tokenWrapped.transferFrom(msg.sender, address(this), amount); + IERC20Upgradeable(address(tokenWrapped)).safeTransferFrom(msg.sender, address(this), amount); } else { // Burn tokens tokenWrapped.burn(msg.sender, amount); @@ -114,8 +112,7 @@ contract BridgeL2SovereignChain is // If is not mintable transfer instead of mint if (wrappedAddressIsNotMintable[address(tokenWrapped)]) { // Transfer wETH - // q: safe transfer? - tokenWrapped.transfer(destinationAddress, amount); + IERC20Upgradeable(address(tokenWrapped)).safeTransfer(destinationAddress, amount); } else { // Claim wETH tokenWrapped.mint(destinationAddress, amount); @@ -180,11 +177,21 @@ contract BridgeL2SovereignChain is /** * @notice Remove the address of a remapped token from the mapping * @notice It also removes the token from the isNotMintable mapping + * @notice Altough the token is removed from the mapping, the user will still be able to withdraw their tokens using tokenInfoToWrappedToken mapping * @param sovereignTokenAddress Address of the sovereign wrapped token */ function removeSovereignTokenAddress( address sovereignTokenAddress ) external onlyBridgeManager { + // Only allow to remove already mapped tokens + TokenInformation memory tokenInfo = wrappedTokenToTokenInfo[sovereignTokenAddress]; + bytes32 tokenInfoHash = keccak256( + abi.encodePacked(tokenInfo.originNetwork, tokenInfo.originTokenAddress) + ); + + if(tokenInfoToWrappedToken[tokenInfoHash] == address(0)) { + revert TokenNotMapped(); + } delete wrappedTokenToTokenInfo[sovereignTokenAddress]; delete wrappedAddressIsNotMintable[sovereignTokenAddress]; } diff --git a/contracts/v2/sovereignChains/GlobalExitRootManagerL2SovereignChain.sol b/contracts/v2/sovereignChains/GlobalExitRootManagerL2SovereignChain.sol index b53e905c..f3d33899 100644 --- a/contracts/v2/sovereignChains/GlobalExitRootManagerL2SovereignChain.sol +++ b/contracts/v2/sovereignChains/GlobalExitRootManagerL2SovereignChain.sol @@ -1,14 +1,13 @@ // SPDX-License-Identifier: AGPL-3.0 pragma solidity 0.8.20; -import "../../interfaces/IBasePolygonZkEVMGlobalExitRoot.sol"; import {PolygonAccessControlUpgradeable} from "../lib/PolygonAccessControlUpgradeable.sol"; -import "../PolygonZkEVMGlobalExitRootV2.sol"; +import "../../PolygonZkEVMGlobalExitRootL2.sol"; /** * Contract responsible for managing the exit roots for the Sovereign chains and global exit roots */ -contract GlobalExitRootManagerL2SovereignChain is PolygonZkEVMGlobalExitRootV2 { +contract GlobalExitRootManagerL2SovereignChain is PolygonZkEVMGlobalExitRootL2 { /** * @dev Emitted when a new global exit root is inserted */ @@ -24,33 +23,12 @@ contract GlobalExitRootManagerL2SovereignChain is PolygonZkEVMGlobalExitRootV2 { _; } - /** - * @notice Only allows a function to be callable by the bride contract - */ - modifier onlyBridgeAddress() { - if (msg.sender != bridgeAddress) { - revert OnlyAllowedContracts(); - } - _; - } - /** * @param _bridgeAddress PolygonZkEVMBridge contract address */ constructor( - address _rollupManager, address _bridgeAddress - ) PolygonZkEVMGlobalExitRootV2(_rollupManager, _bridgeAddress) {} - - /** - * @notice Update the exit root of one of the networks and the global exit root - * @param newRoot new exit tree root - */ - function updateExitRoot( - bytes32 newRoot - ) external override onlyBridgeAddress { - lastRollupExitRoot = newRoot; - } + ) PolygonZkEVMGlobalExitRootL2( _bridgeAddress) {} /** * @notice Insert a new global exit root diff --git a/test/contractsv2/BridgeL2GasTokensSovereignChains.test.ts b/test/contractsv2/BridgeL2GasTokensSovereignChains.test.ts index f0fdadd1..d3315b19 100644 --- a/test/contractsv2/BridgeL2GasTokensSovereignChains.test.ts +++ b/test/contractsv2/BridgeL2GasTokensSovereignChains.test.ts @@ -76,7 +76,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { "GlobalExitRootManagerL2SovereignChain" ); sovereignChainGlobalExitRoot = await SovereignChainGlobalExitRootFactory.deploy( - rollupManager.address, sovereignChainBridgeContract.target ); @@ -274,8 +273,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { await sovereignChainBridgeContract.verifyMerkleProof(leafValue, proof, index, rootSCMainnet) ).to.be.equal(true); - const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rootJSSovereignRollup); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); }); it("should PolygonZkEVMBridge message and verify merkle proof", async () => { @@ -380,9 +377,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { await sovereignChainBridgeContract.verifyMerkleProof(leafValue, proof, index, rootSCSovereignChain) ).to.be.equal(true); - const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rootJSSovereignChain); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); - // bridge message without value is fine await expect( sovereignChainBridgeContract.bridgeMessage(destinationNetwork, destinationAddress, true, metadata, {}) @@ -463,7 +457,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { balanceBridge + amount ); expect(await sovereignChainBridgeContract.lastUpdatedDepositCount()).to.be.equal(0); - expect(await sovereignChainGlobalExitRoot.lastMainnetExitRoot()).to.be.equal(ethers.ZeroHash); + expect(mainnetExitRoot).to.be.equal(ethers.ZeroHash); // check merkle root with SC const rootSCSovereignChain = await sovereignChainBridgeContract.getRoot(); @@ -475,10 +469,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { // no state changes since there are not any deposit pending to be updated await sovereignChainBridgeContract.updateGlobalExitRoot(); expect(await sovereignChainBridgeContract.lastUpdatedDepositCount()).to.be.equal(1); - expect(await sovereignChainGlobalExitRoot.lastMainnetExitRoot()).to.be.equal(mainnetExitRoot); - - const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rootJSSovereignChain); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); + expect(mainnetExitRoot).to.be.equal(mainnetExitRoot); // bridge message await expect( @@ -513,13 +504,13 @@ describe("SovereignChainBridge Gas tokens tests", () => { 1 ); expect(await sovereignChainBridgeContract.lastUpdatedDepositCount()).to.be.equal(1); - expect(await sovereignChainGlobalExitRoot.lastMainnetExitRoot()).to.be.equal(mainnetExitRoot); + expect(mainnetExitRoot).to.be.equal(mainnetExitRoot); // Update global exit root await sovereignChainBridgeContract.updateGlobalExitRoot(); expect(await sovereignChainBridgeContract.lastUpdatedDepositCount()).to.be.equal(2); - expect(await sovereignChainGlobalExitRoot.lastMainnetExitRoot()).to.not.be.equal(rootJSSovereignChain); + expect(mainnetExitRoot).to.not.be.equal(rootJSSovereignChain); // Just to have the metric of a low cost bridge Asset const tokenAddress2 = WETHToken.target; // Ether @@ -604,11 +595,10 @@ describe("SovereignChainBridge Gas tokens tests", () => { // check roots const sovereignChainExitRootSC = await sovereignChainGlobalExitRoot.lastRollupExitRoot(); expect(sovereignChainExitRootSC).to.be.equal(rootRollup); - const mainnetExitRootSC = await sovereignChainGlobalExitRoot.lastMainnetExitRoot(); + const mainnetExitRootSC = ethers.ZeroHash; expect(mainnetExitRootSC).to.be.equal(mainnetExitRoot); const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rootRollup); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); // Insert global exit root expect(await sovereignChainGlobalExitRoot.insertGlobalExitRoot(computedGlobalExitRoot)) @@ -708,7 +698,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { const metadata = metadataToken; const metadataHash = ethers.solidityPackedKeccak256(["bytes"], [metadata]); - const mainnetExitRoot = await sovereignChainGlobalExitRoot.lastMainnetExitRoot(); + const mainnetExitRoot = ethers.ZeroHash; // compute root merkle tree in Js const height = 32; @@ -751,7 +741,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { expect(rollupExitRootSC).to.be.equal(rootRollup); const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rollupExitRootSC); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); // Insert global exit root expect(await sovereignChainGlobalExitRoot.insertGlobalExitRoot(computedGlobalExitRoot)) @@ -982,10 +971,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { rootSCSovereignChain ) ).to.be.equal(true); - - const rollupExitRoot2 = await sovereignChainGlobalExitRoot.lastRollupExitRoot(); - const computedGlobalExitRoot2 = calculateGlobalExitRoot(rootJSMainnet, rollupExitRoot2); - expect(computedGlobalExitRoot2).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); }); it("should PolygonZkEVMBridge and sync the current root with events", async () => { @@ -996,8 +981,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { const destinationNetwork = networkIDRollup; const destinationAddress = deployer.address; - const metadata = "0x"; // since is ether does not have metadata - // create 3 new deposit await expect( sovereignChainBridgeContract.bridgeAsset( @@ -1116,7 +1099,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { const metadata = metadataToken; const metadataHash = ethers.solidityPackedKeccak256(["bytes"], [metadata]); - const mainnetExitRoot = await sovereignChainGlobalExitRoot.lastMainnetExitRoot(); + const mainnetExitRoot = ethers.ZeroHash; // compute root merkle tree in Js const height = 32; @@ -1150,7 +1133,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { expect(rollupExitRootSC).to.be.equal(rollupRoot); const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rollupExitRootSC); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); // Insert global exit root expect(await sovereignChainGlobalExitRoot.insertGlobalExitRoot(computedGlobalExitRoot)) .to.emit(sovereignChainGlobalExitRoot, "InsertGlobalExitRoot") @@ -1268,7 +1250,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { const metadata = "0x"; // since is ether does not have metadata const metadataHash = ethers.solidityPackedKeccak256(["bytes"], [metadata]); - const mainnetExitRoot = await sovereignChainGlobalExitRoot.lastMainnetExitRoot(); + const mainnetExitRoot = ethers.ZeroHash; // compute root merkle tree in Js const height = 32; @@ -1300,7 +1282,6 @@ describe("SovereignChainBridge Gas tokens tests", () => { expect(rollupExitRootSC).to.be.equal(rollupRoot); const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rollupExitRootSC); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); // Insert global exit root expect(await sovereignChainGlobalExitRoot.insertGlobalExitRoot(computedGlobalExitRoot)) .to.emit(sovereignChainGlobalExitRoot, "InsertGlobalExitRoot") @@ -1372,7 +1353,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { const metadata = "0x176923791298713271763697869132"; // since is ether does not have metadata const metadataHash = ethers.solidityPackedKeccak256(["bytes"], [metadata]); - const mainnetExitRoot = await sovereignChainGlobalExitRoot.lastMainnetExitRoot(); + const mainnetExitRoot = ethers.ZeroHash; // compute root merkle tree in Js const height = 32; @@ -1404,7 +1385,7 @@ describe("SovereignChainBridge Gas tokens tests", () => { expect(rollupExitRootSC).to.be.equal(rollupRoot); const computedGlobalExitRoot = calculateGlobalExitRoot(mainnetExitRoot, rollupExitRootSC); - expect(computedGlobalExitRoot).to.be.equal(await sovereignChainGlobalExitRoot.getLastGlobalExitRoot()); + // Insert global exit root expect(await sovereignChainGlobalExitRoot.insertGlobalExitRoot(computedGlobalExitRoot)) .to.emit(sovereignChainGlobalExitRoot, "InsertGlobalExitRoot") diff --git a/test/contractsv2/BridgeL2SovereignChain.test.ts b/test/contractsv2/BridgeL2SovereignChain.test.ts index b466e58e..c1639d97 100644 --- a/test/contractsv2/BridgeL2SovereignChain.test.ts +++ b/test/contractsv2/BridgeL2SovereignChain.test.ts @@ -67,7 +67,6 @@ describe("BridgeL2SovereignChain Contract", () => { "GlobalExitRootManagerL2SovereignChain" ); sovereignChainGlobalExitRoot = await PolygonZkEVMGlobalExitRootFactory.deploy( - rollupManager.address, sovereignChainBridgeContract.target );