Review fixes

0xPolygonHermez · Sep 17, 2024 · 0dc632d · 0dc632d
1 parent 8f697e2
commit 0dc632d
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 64 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -67,7 +67,6 @@ describe("BridgeL2SovereignChain Contract", () => {
             "GlobalExitRootManagerL2SovereignChain"
         );
         sovereignChainGlobalExitRoot = await PolygonZkEVMGlobalExitRootFactory.deploy(
-            rollupManager.address,
             sovereignChainBridgeContract.target
         );