refactor and test and chore: audit reaction

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);
     }
@@ -95,10 +94,13 @@ contract FxERC20ChildTunnel is FxBaseChildTunnel {
         }
 
         // Deposit tokens on an L2 bridge contract (lock)
-        IERC20(childToken).transferFrom(msg.sender, address(this), amount);
+        bool success = IERC20(childToken).transferFrom(msg.sender, address(this), amount);
+        if (!success) {
+            revert TransferFailed(rootToken, 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);
 

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;
+    }
+}

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

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");
+        });
+    });
 });