diff --git a/README.md b/README.md index 6d83da0..fb87838 100644 --- a/README.md +++ b/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). diff --git a/audits/internal7/README.md b/audits/internal7/README.md index 185dda4..936b9d6 100644 --- a/audits/internal7/README.md +++ b/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.
+[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 -``` \ No newline at end of file +``` +[x] fixed \ No newline at end of file diff --git a/contracts/bridges/BridgedERC20.sol b/contracts/bridges/BridgedERC20.sol index 459fe98..7aff64d 100644 --- a/contracts/bridges/BridgedERC20.sol +++ b/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; } diff --git a/contracts/bridges/FxERC20ChildTunnel.sol b/contracts/bridges/FxERC20ChildTunnel.sol index 82a48fc..da879bd 100644 --- a/contracts/bridges/FxERC20ChildTunnel.sol +++ b/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 - /// @author Andrey Lebedev - @@ -57,6 +64,7 @@ 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( @@ -64,23 +72,14 @@ contract FxERC20ChildTunnel is FxBaseChildTunnel { 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); } } diff --git a/contracts/bridges/FxERC20RootTunnel.sol b/contracts/bridges/FxERC20RootTunnel.sol index 6fd8724..713e41c 100755 --- a/contracts/bridges/FxERC20RootTunnel.sol +++ b/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 - /// @author Andrey Lebedev - @@ -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); } } diff --git a/contracts/test/BrokenERC20.sol b/contracts/test/BrokenERC20.sol new file mode 100644 index 0000000..9642b77 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/docs/deployment.md b/docs/deployment.md index c92288f..b42e9d7 100644 --- a/docs/deployment.md +++ b/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). \ No newline at end of file +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)`. \ No newline at end of file diff --git a/scripts/deployment/README.md b/scripts/deployment/README.md index 6781d4d..fe5e0c1 100644 --- a/scripts/deployment/README.md +++ b/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 +``` diff --git a/scripts/deployment/bridges/polygon/deploy_04_set_root_tunnel.js b/scripts/deployment/bridges/polygon/deploy_04_set_root_tunnel.js new file mode 100644 index 0000000..6a4ccce --- /dev/null +++ b/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); + }); diff --git a/scripts/deployment/deploy_19_bridged_erc20.js b/scripts/deployment/deploy_19_bridged_erc20.js index 1aad9a4..d4948d6 100644 --- a/scripts/deployment/deploy_19_bridged_erc20.js +++ b/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 diff --git a/scripts/deployment/deploy_21_bridged_erc20_change_owner.js b/scripts/deployment/deploy_21_22_bridged_erc20_change_owners.js similarity index 68% rename from scripts/deployment/deploy_21_bridged_erc20_change_owner.js rename to scripts/deployment/deploy_21_22_bridged_erc20_change_owners.js index 612d849..aea3eae 100644 --- a/scripts/deployment/deploy_21_bridged_erc20_change_owner.js +++ b/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() diff --git a/test/FxERC20.js b/test/FxERC20.js index f26451b..cfc1471 100644 --- a/test/FxERC20.js +++ b/test/FxERC20.js @@ -6,8 +6,10 @@ const { ethers } = require("hardhat"); describe("FxERC20", function () { let fxERC20RootTunnel; let fxERC20ChildTunnel; + let fxRootMock; let childToken; let rootToken; + let brokenToken; let signers; let deployer; const AddressZero = ethers.constants.AddressZero; @@ -26,12 +28,12 @@ describe("FxERC20", function () { // Root token is a bridged ERC20 token const BridgedToken = await ethers.getContractFactory("BridgedERC20"); - rootToken = await BridgedToken.deploy("Bridged token", "BERC20"); + rootToken = await BridgedToken.deploy("Bridged token", "BERC20", 18); await rootToken.deployed(); // ERC20 tunnels const FxRootMock = await ethers.getContractFactory("FxRootMock"); - const fxRootMock = await FxRootMock.deploy(); + fxRootMock = await FxRootMock.deploy(); await fxRootMock.deployed(); const FxERC20RootTunnel = await ethers.getContractFactory("FxERC20RootTunnel"); @@ -54,6 +56,11 @@ describe("FxERC20", function () { // Mint tokens await childToken.mint(deployer.address, initMint); + + // Broken ERC20 token + const BrokenToken = await ethers.getContractFactory("BrokenERC20"); + brokenToken = await BrokenToken.deploy(); + await brokenToken.deployed(); }); context("Initialization", async function () { @@ -266,4 +273,73 @@ describe("FxERC20", function () { expect(balanceDiff).to.equal(amount); }); }); + + context("Deposit and withdraw with broken ERC20 tokens", async function () { + it("Should fail when trying to deposit", async function () { + const FxERC20ChildTunnel = await ethers.getContractFactory("FxERC20ChildTunnel"); + const brokenChildTunnel = await FxERC20ChildTunnel.deploy(deployer.address, brokenToken.address, rootToken.address); + await brokenChildTunnel.deployed(); + + // Mint tokens to deployer + await brokenToken.mint(deployer.address, amount); + + // Approve tokens + await brokenToken.connect(deployer).approve(brokenChildTunnel.address, amount); + + // Send tokens to L1 + await expect( + brokenChildTunnel.connect(deployer).deposit(amount) + ).to.be.revertedWithCustomError(fxERC20ChildTunnel, "TransferFailed"); + }); + + it("Should fail when trying to withdraw on the root side", async function () { + const FxERC20RootTunnel = await ethers.getContractFactory("FxERC20RootTunnel"); + const brokenRootTunnel = await FxERC20RootTunnel.deploy(deployer.address, fxRootMock.address, + childToken.address, brokenToken.address); + await brokenRootTunnel.deployed(); + + // Approve tokens + await childToken.approve(fxERC20ChildTunnel.address, amount); + + // Send tokens to L1 + await fxERC20ChildTunnel.connect(deployer).deposit(amount); + + // Set child tunnel accordingly + await brokenRootTunnel.setFxChildTunnel(fxERC20ChildTunnel.address); + + // Approve tokens + await childToken.approve(brokenRootTunnel.address, amount); + + // Send tokens to L2 + await expect( + brokenRootTunnel.connect(deployer).withdraw(amount) + ).to.be.revertedWithCustomError(brokenRootTunnel, "TransferFailed"); + }); + + it("Should fail when trying to withdraw on the child side", async function () { + const FxERC20ChildTunnel = await ethers.getContractFactory("FxERC20ChildTunnel"); + const brokenChildTunnel = await FxERC20ChildTunnel.deploy(fxRootMock.address, brokenToken.address, rootToken.address); + await brokenChildTunnel.deployed(); + + const FxERC20RootTunnel = await ethers.getContractFactory("FxERC20RootTunnel"); + const brokenRootTunnel = await FxERC20RootTunnel.deploy(deployer.address, fxRootMock.address, + brokenToken.address, rootToken.address); + await brokenRootTunnel.deployed(); + + // Set the root tunnel in the root mock + await fxRootMock.setRootTunnel(brokenRootTunnel.address); + + // Set child and root tunnels accordingly + await brokenRootTunnel.setFxChildTunnel(brokenChildTunnel.address); + await brokenChildTunnel.setFxRootTunnel(brokenRootTunnel.address); + + // Approve tokens + await rootToken.approve(brokenRootTunnel.address, amount); + + // Send tokens to L2 + await expect( + brokenRootTunnel.connect(deployer).withdraw(amount) + ).to.be.revertedWithCustomError(brokenChildTunnel, "TransferFailed"); + }); + }); });