Merge pull request #97 from valory-xyz/audit_reaction

refactor and test and chore: audit reaction

README.md

@@ -158,6 +158,9 @@ The description of bridge-related deployment procedure is very similar to the or
 - [bridges-polygon](https://github.com/valory-xyz/autonolas-governance/blob/main/scripts/deployment/bridges/polygon);
 - [bridges-gnosis](https://github.com/valory-xyz/autonolas-governance/blob/main/scripts/deployment/bridges/gnosis).
 
+The description of ERC20 token bridging deployment between Polygon and Ethereum can be found here:
+[deployment](https://github.com/valory-xyz/autonolas-governance/blob/main/scripts/deployment).
+
 ## Documents
 All the project-related documents are located here: [docs](https://github.com/valory-xyz/autonolas-governance/blob/main/docs).
 

audits/internal7/README.md

@@ -41,6 +41,7 @@ ERC20(_name, _symbol, 18)
 
 decimals (18) as constant is OK?
 ```
+[x] fixed
 
 #### Reentrancy (not critical since the token is trusted, but it’s better to fix the problem)
 ```solidity
@@ -61,6 +62,7 @@ function depositTo(address to, uint256 amount) external {
     
 ```
 Notes: It is better to use explicit protection like reentrancy_guard_variable than CEI. <br>
+[x] fixed
 
 #### Non-checked return
 ```solidity
@@ -69,6 +71,7 @@ unchecked-transfer
         IERC20(childToken).transfer(to, amount);
 
 ```
+[x] fixed
 
 #### Non-checked return
 ```solidity
@@ -77,6 +80,7 @@ unchecked-transfer
         IERC20(rootToken).transferFrom(msg.sender, address(this), amount);
 
 ```
+[x] fixed
 
 #### Overengineering
 ```solidity
@@ -88,4 +92,5 @@ https://github.com/pessimistic-io/slitherin/blob/master/docs/dubious_typecast.md
 +
 no-inline-assembly
 I'm not sure that it is necessary to parse using assembler
-```
+```
+[x] fixed

contracts/bridges/BridgedERC20.sol

@@ -21,7 +21,7 @@ contract BridgedERC20 is ERC20 {
     // Bridged token owner
     address public owner;
 
-    constructor(string memory _name, string memory _symbol) ERC20(_name, symbol, 18) {
+    constructor(string memory _name, string memory _symbol, uint8 _decimals) ERC20(_name, _symbol, _decimals) {
         owner = msg.sender;
     }
 

contracts/bridges/FxERC20ChildTunnel.sol

@@ -10,6 +10,13 @@ error ZeroAddress();
 /// @dev Zero value when it has to be different from zero.
 error ZeroValue();
 
+/// @dev Failure of a transfer.
+/// @param token Address of a token.
+/// @param from Address `from`.
+/// @param to Address `to`.
+/// @param amount Token amount.
+error TransferFailed(address token, address from, address to, uint256 amount);
+
 /// @title FxERC20ChildTunnel - Smart contract for the L2 token management part
 /// @author Aleksandr Kuperman - <[email protected]>
 /// @author Andrey Lebedev - <[email protected]>
@@ -57,30 +64,22 @@ contract FxERC20ChildTunnel is FxBaseChildTunnel {
     }
 
     /// @dev Receives the token message from L1 and transfers L2 tokens to a specified address.
+    /// @notice Reentrancy is not possible as tokens are verified before the contract deployment.
     /// @param sender FxERC20RootTunnel contract address from L1.
     /// @param message Incoming bridge message.
     function _processMessageFromRoot(
         uint256 /* stateId */,
         address sender,
         bytes memory message
     ) internal override validateSender(sender) {
-        // Decode incoming message from root: (address, address, uint96)
-        address from;
-        address to;
-        // The token amount is limited to be no bigger than 2^96 - 1
-        uint96 amount;
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            // Offset 20 bytes for the address from (160 bits)
-            from := mload(add(message, 20))
-            // Offset 20 bytes for the address to (160 bits)
-            to := mload(add(message, 40))
-            // Offset 12 bytes of amount (96 bits)
-            amount := mload(add(message, 52))
-        }
+        // Decode incoming message from root: (address, address, uint256)
+        (address from, address to, uint256 amount) = abi.decode(message, (address, address, uint256));
 
         // Transfer decoded amount of tokens to a specified address
-        IERC20(childToken).transfer(to, amount);
+        bool success = IERC20(childToken).transfer(to, amount);
+        if (!success) {
+            revert TransferFailed(childToken, address(this), to, amount);
+        }
 
         emit FxWithdrawERC20(rootToken, childToken, from, to, amount);
     }
@@ -94,14 +93,17 @@ contract FxERC20ChildTunnel is FxBaseChildTunnel {
             revert ZeroValue();
         }
 
-        // Deposit tokens on an L2 bridge contract (lock)
-        IERC20(childToken).transferFrom(msg.sender, address(this), amount);
-
-        // Encode message for root: (address, address, uint96)
-        bytes memory message = abi.encodePacked(msg.sender, to, uint96(amount));
+        // Encode message for root: (address, address, uint256)
+        bytes memory message = abi.encode(msg.sender, to, amount);
         // Send message to root
         _sendMessageToRoot(message);
 
+        // Deposit tokens on an L2 bridge contract (lock)
+        bool success = IERC20(childToken).transferFrom(msg.sender, address(this), amount);
+        if (!success) {
+            revert TransferFailed(childToken, msg.sender, address(this), amount);
+        }
+
         emit FxDepositERC20(childToken, rootToken, msg.sender, to, amount);
     }
 }

contracts/bridges/FxERC20RootTunnel.sol

@@ -10,6 +10,13 @@ error ZeroAddress();
 /// @dev Zero value when it has to be different from zero.
 error ZeroValue();
 
+/// @dev Failure of a transfer.
+/// @param token Address of a token.
+/// @param from Address `from`.
+/// @param to Address `to`.
+/// @param amount Token amount.
+error TransferFailed(address token, address from, address to, uint256 amount);
+
 /// @title FxERC20RootTunnel - Smart contract for the L1 token management part
 /// @author Aleksandr Kuperman - <[email protected]>
 /// @author Andrey Lebedev - <[email protected]>
@@ -63,20 +70,8 @@ contract FxERC20RootTunnel is FxBaseRootTunnel {
     /// @dev Receives the token message from L2 and transfers bridged tokens to a specified address.
     /// @param message Incoming bridge message.
     function _processMessageFromChild(bytes memory message) internal override {
-        // Decode incoming message from child: (address, address, uint96)
-        address from;
-        address to;
-        // The token amount is limited to be no bigger than 2^96 - 1
-        uint96 amount;
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            // Offset 20 bytes for the address from (160 bits)
-            from := mload(add(message, 20))
-            // Offset 20 bytes for the address to (160 bits)
-            to := mload(add(message, 40))
-            // Offset 12 bytes of amount (96 bits)
-            amount := mload(add(message, 52))
-        }
+        // Decode incoming message from child: (address, address, uint256)
+        (address from, address to, uint256 amount) = abi.decode(message, (address, address, uint256));
 
         // Mints bridged amount of tokens to a specified address
         IERC20(rootToken).mint(to, amount);
@@ -85,6 +80,7 @@ contract FxERC20RootTunnel is FxBaseRootTunnel {
     }
 
     /// @dev Withdraws bridged tokens from L1 to get their original tokens on L1 by a specified address.
+    /// @notice Reentrancy is not possible as tokens are verified before the contract deployment.
     /// @param to Destination address on L2.
     /// @param amount Token amount to be withdrawn.
     function _withdraw(address to, uint256 amount) internal {
@@ -93,17 +89,20 @@ contract FxERC20RootTunnel is FxBaseRootTunnel {
             revert ZeroValue();
         }
 
+        // Encode message for child: (address, address, uint256)
+        bytes memory message = abi.encode(msg.sender, to, amount);
+        // Send message to child
+        _sendMessageToChild(message);
+
         // Transfer tokens from sender to this contract address
-        IERC20(rootToken).transferFrom(msg.sender, address(this), amount);
+        bool success = IERC20(rootToken).transferFrom(msg.sender, address(this), amount);
+        if (!success) {
+            revert TransferFailed(rootToken, msg.sender, address(this), amount);
+        }
 
         // Burn bridged tokens
         IERC20(rootToken).burn(amount);
 
-        // Encode message for child: (address, address, uint96)
-        bytes memory message = abi.encodePacked(msg.sender, to, uint96(amount));
-        // Send message to child
-        _sendMessageToChild(message);
-
         emit FxWithdrawERC20(rootToken, childToken, msg.sender, to, amount);
     }
 }

contracts/test/BrokenERC20.sol

@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.21;
+
+import {ERC20} from "../../lib/solmate/src/tokens/ERC20.sol";
+
+/// @dev Only `owner` has a privilege, but the `sender` was provided.
+/// @param sender Sender address.
+/// @param owner Required sender address as an owner.
+error OwnerOnly(address sender, address owner);
+
+/// @dev Provided zero address.
+error ZeroAddress();
+
+
+/// @title BrokenERC20 - Smart contract for an ERC20 token with a broken functionality
+contract BrokenERC20 is ERC20 {
+
+    constructor() ERC20("Broken ERC20", "BRERC20", 18)
+    {}
+
+    /// @dev Mints tokens.
+    /// @param account Account address.
+    /// @param amount Token amount.
+    function mint(address account, uint256 amount) external {
+        _mint(account, amount);
+    }
+
+    /// @dev Burns tokens.
+    /// @param amount Token amount to burn.
+    function burn(uint256 amount) external {
+        _burn(msg.sender, amount);
+    }
+
+    /// @dev Broken transfer function.
+    function transfer(address, uint256) public virtual override returns (bool) {
+        return false;
+    }
+
+    /// @dev Broken transferFrom function.
+    function transferFrom(address, address, uint256) public virtual override returns (bool) {
+        return false;
+    }
+}

docs/deployment.md

@@ -24,4 +24,12 @@
 ## Steps of deploying supplemental contracts.
 16. EOA to deploy wveOLAS contract pointed to veOLAS and OLAS;
 17. EOA to deploy new GovernorOLAS contract with wveOLAS and Timelock addresses as input parameters and other defined governor-related parameters;
-18. Timelock to revoke admin ("TIMELOCK_ADMIN_ROLE"), proposer ("PROPOSER_ROLE"), executor ("EXECUTOR_ROLE"), and canceller ("CANCELLER_ROLE") roles from original GovernorOLAS, give admin ("TIMELOCK_ADMIN_ROLE"), proposer ("PROPOSER_ROLE"), executor ("EXECUTOR_ROLE"), and canceller ("CANCELLER_ROLE") roles to a new GovernorOLAS based on wveOLAS (via voting).
+18. Timelock to revoke admin ("TIMELOCK_ADMIN_ROLE"), proposer ("PROPOSER_ROLE"), executor ("EXECUTOR_ROLE"), and canceller ("CANCELLER_ROLE") roles from original GovernorOLAS, give admin ("TIMELOCK_ADMIN_ROLE"), proposer ("PROPOSER_ROLE"), executor ("EXECUTOR_ROLE"), and canceller ("CANCELLER_ROLE") roles to a new GovernorOLAS based on wveOLAS (via voting).
+
+## Steps of deploying Polygon-Ethereum ERC20 token bridging contracts.
+1. EOA to deploy BridgedERC20 contract on Ethereum;
+2. EOA to deploy FxERC20ChildTunnel contract on Polygon specifying both child (Polygon) and root (BridgedERC20 on Ethereum) tokens;
+3. EOA to deploy FxERC20RootTunnel contract on Ethereum specifying both child (Polygon) and root (BridgedERC20 on Ethereum) tokens;
+4. EOA to change owner of BridgedERC20 to FxERC20RootTunnel by calling `changeOwner(FxERC20RootTunnel)`;
+5. FxERC20RootTunnel to set child tunnel by calling `setFxChildTunnel(FxERC20ChildTunnel)`;
+6. FxERC20ChildTunnel to set root tunnel by calling `setFxRootTunnel(FxERC20RootTunnel)`.

scripts/deployment/README.md

@@ -61,6 +61,16 @@ npx hardhat run scripts/deployment/deploy_17_governorTwo.js --network network_ty
 Then, after successful deployment of two supplemental contracts, the last script gives the proposal payload necessary to finalize the deployment:
 `npx hardhat run scripts/deployment/deploy_18_governor_to_governorTwo.js --network network_type`.
 
+## Deployment of Polygon-Ethereum ERC20 bridging contracts
+For deploying ERC20 bridging contracts listed in [deployment.md](https://github.com/valory-xyz/autonolas-governance/blob/main/docs/deployment.md),
+run the following scripts:
+```
+npx hardhat run scripts/deployment/deploy_19_bridged_erc20.js --network mainnet
+npx hardhat run scripts/deployment/deploy_03_erc20_child_tunnel.js --network polygon
+npx hardhat run scripts/deployment/deploy_20_erc20_root_tunnel.js --network mainnet
+npx hardhat run scripts/deployment/deploy_21_22_bridged_erc20_change_owners.js --network mainnet
+npx hardhat run scripts/deployment/bridges/polygon/deploy_04_set_root_tunnel.js --network polygon
+```
 
 
 

scripts/deployment/bridges/polygon/deploy_04_set_root_tunnel.js

@@ -0,0 +1,55 @@
+/*global process*/
+
+const { ethers } = require("hardhat");
+const { LedgerSigner } = require("@anders-t/ethers-ledger");
+
+async function main() {
+    const fs = require("fs");
+    const globalsFile = "globals.json";
+    const dataFromJSON = fs.readFileSync(globalsFile, "utf8");
+    let parsedData = JSON.parse(dataFromJSON);
+    const useLedger = parsedData.useLedger;
+    const derivationPath = parsedData.derivationPath;
+    const providerName = parsedData.providerName;
+    const fxERC20ChildTunnelAddress = parsedData.fxERC20ChildTunnelAddress;
+    const fxERC20RootTunnelAddress = parsedData.fxERC20RootTunnelAddress;
+    let EOA;
+
+    let provider;
+    if (providerName == "polygon") {
+        provider = await ethers.providers.getDefaultProvider("matic");
+    } else {
+        const mumbaiURL = "https://polygon-mumbai.g.alchemy.com/v2/" + process.env.ALCHEMY_API_KEY_MUMBAI;
+        provider = new ethers.providers.JsonRpcProvider(mumbaiURL);
+    }
+    const signers = await ethers.getSigners();
+
+    if (useLedger) {
+        EOA = new LedgerSigner(provider, derivationPath);
+    } else {
+        EOA = signers[0];
+    }
+    // EOA address
+    const deployer = await EOA.getAddress();
+    console.log("EOA is:", deployer);
+
+    // Transaction signing and execution
+    console.log("4. FxERC20ChildTunnel to set root tunnel to FxERC20RootTunnel");
+    const fxERC20ChildTunnel = await ethers.getContractAt("FxERC20ChildTunnel", fxERC20ChildTunnelAddress);
+    console.log("You are signing the following transaction: FxERC20ChildTunnel.connect(EOA).setFxRootTunnel(FxERC20RootTunnel)");
+    const gasPriceInGwei = "230";
+    const gasPrice = ethers.utils.parseUnits(gasPriceInGwei, "gwei");
+    const result = await fxERC20ChildTunnel.setFxRootTunnel(fxERC20RootTunnelAddress, { gasPrice });
+
+    // Transaction details
+    console.log("Contract deployment: FxERC20ChildTunnel");
+    console.log("Contract address:", fxERC20ChildTunnel.address);
+    console.log("Transaction:", result.hash);
+}
+
+main()
+    .then(() => process.exit(0))
+    .catch((error) => {
+        console.error(error);
+        process.exit(1);
+    });

scripts/deployment/deploy_19_bridged_erc20.js

@@ -30,7 +30,7 @@ async function main() {
     const BridgedERC20 = await ethers.getContractFactory("BridgedERC20");
     console.log("You are signing the following transaction: BridgedERC20.connect(EOA).deploy()");
     const bridgedERC20 = await BridgedERC20.connect(EOA).deploy();
-    //const bridgedERC20 = await BridgedERC20.connect(EOA).deploy("ERC20 bridged token", "BridgedERC20");
+    //const bridgedERC20 = await BridgedERC20.connect(EOA).deploy("ERC20 bridged token", "BridgedERC20", 18);
     let result = await bridgedERC20.deployed();
 
     // Transaction details

scripts/deployment/deploy_21_bridged_erc20_change_owner.js → scripts/deployment/deploy_21_22_bridged_erc20_change_owners.js

@@ -11,8 +11,9 @@ async function main() {
     const useLedger = parsedData.useLedger;
     const derivationPath = parsedData.derivationPath;
     const providerName = parsedData.providerName;
-    const fxERC20RootTunnelAddress = parsedData.fxERC20RootTunnelAddress;
     const bridgedERC20Address = parsedData.bridgedERC20Address;
+    const fxERC20ChildTunnelAddress = parsedData.fxERC20ChildTunnelAddress;
+    const fxERC20RootTunnelAddress = parsedData.fxERC20RootTunnelAddress;
     let EOA;
 
     const provider = await ethers.providers.getDefaultProvider(providerName);
@@ -31,12 +32,22 @@ async function main() {
     console.log("21. EOA to change owner of BridgedERC20 to FxERC20RootTunnel");
     const bridgedERC20 = await ethers.getContractAt("BridgedERC20", bridgedERC20Address);
     console.log("You are signing the following transaction: bridgedERC20.connect(EOA).changeOwner(FxERC20RootTunnel)");
-    const result = await bridgedERC20.changeOwner(fxERC20RootTunnelAddress);
+    let result = await bridgedERC20.changeOwner(fxERC20RootTunnelAddress);
 
     // Transaction details
     console.log("Contract deployment: BridgedERC20");
     console.log("Contract address:", bridgedERC20.address);
     console.log("Transaction:", result.hash);
+
+    console.log("22. FxERC20RootTunnel to set child tunnel to FxERC20ChildTunnel");
+    const fxERC20RootTunnel = await ethers.getContractAt("FxERC20RootTunnel", fxERC20RootTunnelAddress);
+    console.log("You are signing the following transaction: fxERC20RootTunnel.connect(EOA).setFxChildTunnel(FxERC20ChildTunnel)");
+    result = await fxERC20RootTunnel.setFxChildTunnel(fxERC20ChildTunnelAddress);
+
+    // Transaction details
+    console.log("Contract deployment: FxERC20RootTunnel");
+    console.log("Contract address:", fxERC20RootTunnel.address);
+    console.log("Transaction:", result.hash);
 }
 
 main()