Bridge Updates - Chiliz

In the Solidity programming language, an integer underflow occurs when an integer variable is decremented below its minimum possible value. In Solidity, all integer variables have a fixed size and a fixed range of possible values. For example, a uint8 variable can hold a value between 0 and 255 (inclusive), and a int256 variable can hold a value between -2^255 and 2^255-1 (inclusive).

The deposit function in the contract contains an integer underflow issue when calculating the amountMinusFee variable. This can happen when the value of the "amount" variable is less than the value of the bridgeFee variable. If an integer underflow occurs in the deposit function, the result will be a very large positive number. This can cause problems in the contract, as the large positive value of amountMinusFee will be treated as a valid amount. For example, it could be recorded in the depositRecords mapping and used to update the balance of the depositor.

Code Location

NativeHandler.sol#L91

    function deposit(
        bytes32 resourceID,
        uint8   destinationChainID,
        uint64  depositNonce,
        address depositer,
        bytes   calldata data
    ) external override onlyBridge {
        bytes   memory recipientAddress;
        uint256        amount;
        uint256        lenRecipientAddress;

        assembly {
            amount := calldataload(0xC4)
            recipientAddress := mload(0x40)
            lenRecipientAddress := calldataload(0xE4)
            mstore(0x40, add(0x20, add(recipientAddress, lenRecipientAddress)))

            calldatacopy(
                recipientAddress, // copy to destinationRecipientAddress
                0xE4, // copy from calldata @ 0x104
                sub(calldatasize(), 0xE) // copy size (calldatasize - 0x104)
            )
        }

        address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
        require(_contractWhitelist[tokenAddress], "provided tokenAddress is not whitelisted");

        uint256 bridgeFee = IBridge(_bridgeAddress)._fee();
        uint256 amountMinusFee = amount - bridgeFee;
        require(amountMinusFee > 0, "Invalid amount");

        _depositRecords[destinationChainID][depositNonce] = DepositRecord(
            tokenAddress,
            uint8(lenRecipientAddress),
            destinationChainID,
            resourceID,
            recipientAddress,
            depositer,
            amountMinusFee
        );
    }

SOLVED: The Chiliz team solved the issue by upgrading to pragma 0.8.17 in contracts.

Commit ID: add9f81660e4ab58778f4706e00fc4ac4234a153

If chain IDs are not whitelisted in a smart contract, it may be vulnerable to various types of attacks or errors. A whitelist is a list of approved or allowed values, and in the context of a smart contract, it is used to specify which chain IDs are allowed to interact with the contract.

This could potentially lead to a variety of problems, such as:

• Loss of tokens: If the contract attempts to transfer tokens to an unauthorized chain ID, the tokens may be lost or stolen, and the user will not be able to recover them.

• Invalid state: If the contract attempts to transfer tokens to an unauthorized chain ID, the contract's internal state may become inconsistent or invalid. For example, if the contract maintains a record of all token transfers, it may record an invalid transfer to an unauthorized chain ID, which could cause the contract to malfunction or become unresponsive.

• Denial of service: If an attacker can call the transfer function with an unauthorized chain ID, they may be able to prevent legitimate users from transferring tokens to certain chain IDs. This could potentially cause a denial of service, as users would not be able to transfer tokens to the affected chain IDs.

Overall, a smart contract that does not have a whitelist of allowed chain IDs may be vulnerable to various types of attacks or errors. It is important to include a whitelist of allowed chain IDs in order to ensure that the contract operates correctly and securely.

Code Location

NativeHandler.sol#L91

    function deposit(
        bytes32 resourceID,
        uint8   destinationChainID,
        uint64  depositNonce,
        address depositer,
        bytes   calldata data
    ) external override onlyBridge {
        bytes   memory recipientAddress;
        uint256        amount;
        uint256        lenRecipientAddress;

        assembly {
            amount := calldataload(0xC4)
            recipientAddress := mload(0x40)
            lenRecipientAddress := calldataload(0xE4)
            mstore(0x40, add(0x20, add(recipientAddress, lenRecipientAddress)))

            calldatacopy(
                recipientAddress, // copy to destinationRecipientAddress
                0xE4, // copy from calldata @ 0x104
                sub(calldatasize(), 0xE) // copy size (calldatasize - 0x104)
            )
        }

        address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
        require(_contractWhitelist[tokenAddress], "provided tokenAddress is not whitelisted");

        uint256 bridgeFee = IBridge(_bridgeAddress)._fee();
        uint256 amountMinusFee = amount - bridgeFee;
        require(amountMinusFee > 0, "Invalid amount");

        _depositRecords[destinationChainID][depositNonce] = DepositRecord(
            tokenAddress,
            uint8(lenRecipientAddress),
            destinationChainID,
            resourceID,
            recipientAddress,
            depositer,
            amountMinusFee
        );
    }

contract Tester is DSTest {

     address public user1 = address(0xB0B);
     Bridge internal bridge;
     NativeHandler internal nativeHandler;
     CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
   function setUp() public {
        address [] memory path = new address[](2);
        path[0] = address(this);
        path[1] = address(this);
        bridge = new Bridge(1,path,5,100,100);

        bytes32 [] memory pathByteGG = new bytes32[](2);
        pathByteGG[0] = keccak256(abi.encodePacked("User1"));
        pathByteGG[1] = keccak256(abi.encodePacked("User2"));

        nativeHandler = new NativeHandler(address(bridge),pathByteGG,path,path);

        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.adminSetResource(address(nativeHandler),tokenResource,0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        cheats.deal(user1,1_000_000 ether);
        cheats.deal(address(this), 1_000_000 ether);
   }

   function testExploit() public {
        bytes memory test = "";
        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.deposit{value: 5e2}(anyChainId as uint8 Type, tokenResource, test);
   }
}

SOLVED: The Chiliz team solved the issue by adding a whitelist on chain ids.

Commit ID: 72176b53657bc49d31bfd5f32ef73376505a5935

In the current implementation of the token bridge, the chain-id is used to identify the source and destination chains when transferring tokens. If the same chain-id is used on both sides of the bridge, the system is unable to determine which chain the tokens are coming from and which chain they are going to, and the tokens become stuck.

Code Location

NativeHandler.sol#L91

    function deposit(
        bytes32 resourceID,
        uint8   destinationChainID,
        uint64  depositNonce,
        address depositer,
        bytes   calldata data
    ) external override onlyBridge {
        bytes   memory recipientAddress;
        uint256        amount;
        uint256        lenRecipientAddress;

        assembly {
            amount := calldataload(0xC4)
            recipientAddress := mload(0x40)
            lenRecipientAddress := calldataload(0xE4)
            mstore(0x40, add(0x20, add(recipientAddress, lenRecipientAddress)))

            calldatacopy(
                recipientAddress, // copy to destinationRecipientAddress
                0xE4, // copy from calldata @ 0x104
                sub(calldatasize(), 0xE) // copy size (calldatasize - 0x104)
            )
        }
        address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
        require(_contractWhitelist[tokenAddress], "provided tokenAddress is not whitelisted");

        uint256 bridgeFee = IBridge(_bridgeAddress)._fee();
        uint256 amountMinusFee = amount - bridgeFee;
        require(amountMinusFee > 0, "Invalid amount");

        _depositRecords[destinationChainID][depositNonce] = DepositRecord(
            tokenAddress,
            uint8(lenRecipientAddress),
            destinationChainID,
            resourceID,
            recipientAddress,
            depositer,
            amountMinusFee
        );
    }

contract Tester is DSTest {
     address public user1 = address(0xB0B);
     Bridge internal bridge;
     NativeHandler internal nativeHandler;
     CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
   function setUp() public {
        address [] memory path = new address[](2);
        path[0] = address(this);
        path[1] = address(this);
        bridge = new Bridge(1,path,5,100,100);

        bytes32 [] memory pathByteGG = new bytes32[](2);
        pathByteGG[0] = keccak256(abi.encodePacked("User1"));
        pathByteGG[1] = keccak256(abi.encodePacked("User2"));

        nativeHandler = new NativeHandler(address(bridge),pathByteGG,path,path);

        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.adminSetResource(address(nativeHandler),tokenResource,0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        cheats.deal(user1,1_000_000 ether);
        cheats.deal(address(this), 1_000_000 ether);
   }

   function testExploit() public {
        bytes memory test = "";
        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        ## Chain operates with chain ID as 1.
        bridge.deposit{value: 5e2}(1, tokenResource, test);
   }
}

• Set up a token bridge with the same chain-id on both sides of the bridge.
• Attempt to transfer tokens across the bridge.
• Observe that the tokens become stuck and are not transferred to the destination chain.

SOLVED: The Chiliz team solved the issue by adding the chain id validation.

Commit ID: 72176b53657bc49d31bfd5f32ef73376505a5935

The lack of a quorum definition on the relayer threshold in this system could potentially result in low participation and lack of consensus on important decisions. This is because without a minimum number of participants required to reach consensus, it is possible for some relayers to make decisions that do not reflect the views of the majority of relayers. This can hinder the effectiveness and efficiency of the system, and can lead to conflicts and disputes among relayers. It is important for the system to define a quorum for the relayer threshold to ensure that sufficient participation is achieved and that decisions are made with the support and consensus of the majority of relayers. This can help to promote collaboration and consensus among relayers and improve the functioning and reliability of the system.

Code Location

Bridge.sol#L201

    /**
        @notice Modifies the number of votes required for a proposal to be considered passed.
        @notice Only callable by an address that currently has the admin role.
        @param newThreshold Value {_relayerThreshold} will be changed to.
        @notice Emits {RelayerThresholdChanged} event.
     */
    function adminChangeRelayerThreshold(uint newThreshold) external onlyAdmin {
        _relayerThreshold = newThreshold;
        emit RelayerThresholdChanged(newThreshold);
    }

contract Tester is DSTest {

     address public user1 = address(0xB0B);
     Bridge internal bridge;
     NativeHandler internal nativeHandler;
     CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
   function setUp() public {
        address [] memory path = new address[](2);
        path[0] = address(this);
        path[1] = address(this);
        bridge = new Bridge(1,path,5,100,100);

        bytes32 [] memory pathByteGG = new bytes32[](2);
        pathByteGG[0] = keccak256(abi.encodePacked("User1"));
        pathByteGG[1] = keccak256(abi.encodePacked("User2"));

        nativeHandler = new NativeHandler(address(bridge),pathByteGG,path,path);

        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.adminSetResource(address(nativeHandler),tokenResource,0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        cheats.deal(user1,1_000_000 ether);
        cheats.deal(address(this), 1_000_000 ether);
   }


   function testAdminRelayerThreshold() public {
        bridge.adminChangeRelayerThreshold(0);
   }
}

SOLVED: The Chiliz team solved the issue by adding the quorum relayer threshold definition.

Commit ID: 8a0ffc0bea678f15ec4bcf318740b4170ee3ba95

The ability of the handler to accept payments through any user using the receive() function in Solidity in this system could potentially result in loss of funds. To prevent sending funds through the receive() and fallback() functions in Solidity, you can implement checks and safeguards in your contract to ensure that only payments from specific approved contracts are accepted. This can be done by specifying a list of approved contracts that are allowed to send funds to your contract, and by implementing appropriate checks to verify that the sender of the payment is one of these approved contracts. For example, you can use the msg.sender variable in Solidity to check the sender of the payment, and only accept payments from the approved contracts. You can also use the require() function to enforce these checks and prevent payments from unauthorized contracts from being accepted. It is important to carefully review and test your contract to ensure that it properly implements these checks and safeguards.

Code Location

NativeHandler.sol#L187

    receive() external payable { }

contract Tester is DSTest {

     address public user1 = address(0xB0B);
     Bridge internal bridge;
     NativeHandler internal nativeHandler;
     CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
   function setUp() public {
        address [] memory path = new address[](2);
        path[0] = address(this);
        path[1] = address(this);
        bridge = new Bridge(1,path,5,100,100);

        bytes32 [] memory pathByteGG = new bytes32[](2);
        pathByteGG[0] = keccak256(abi.encodePacked("User1"));
        pathByteGG[1] = keccak256(abi.encodePacked("User2"));

        nativeHandler = new NativeHandler(address(bridge),pathByteGG,path,path);

        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.adminSetResource(address(nativeHandler),tokenResource,0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        cheats.deal(user1,1_000_000 ether);
        cheats.deal(address(this), 1_000_000 ether);
   }

   function testHandlerDepositDirectly() public {
        address(nativeHandler).transfer(5000 ether);
   }
}

SOLVED: The Chiliz team solved the issue by adding a necessary check in the handler.

Commit ID: 49e3c75605632664540b946df8bda7892fc7e884

ERC20Handler contract handles ERC20 deposits and deposit executions. In the given code, the deposit function allows users to deposit native tokens, but it does not include any mechanism for refunding the user if they accidentally send more tokens than required when interacting with ERC20Handler. If a user accidentally sends more native tokens than required, they will lose their excess funds without any way to recover them. This can lead to user dissatisfaction and a loss of trust in the contract. Additionally, the contract may be at risk of attack from users who deliberately attempt to overpay in order to steal the excess funds.

Code Location

Bridge.sol#L308

    function deposit(uint8 destinationChainID, bytes32 resourceID, bytes calldata data) external payable whenNotPaused {
        require(msg.value >= _fee, "invalid value");

        address handler = _resourceIDToHandlerAddress[resourceID];
        require(handler != address(0), "resourceID not mapped to handler");

        uint64 depositNonce = ++_depositCounts[destinationChainID];
        _depositRecords[depositNonce][destinationChainID] = data;

        IDepositExecute depositHandler = IDepositExecute(handler);
        if (msg.value > 0) {
            uint256 valueToSend = msg.value - _fee;
            if (valueToSend > 0) {
                payable(handler).transfer(valueToSend);    
            }
        }
        depositHandler.deposit(resourceID, destinationChainID, depositNonce, msg.sender, data);

        emit Deposit(destinationChainID, resourceID, depositNonce);
    }

   function testRefund() public {
        bytes memory test = "";
        bytes32 tokenResource = keccak256(abi.encodePacked("ERC20"));
        bridge.deposit{value: 5e2}(111, tokenResource, test);
   }

SOLVED: The Chiliz team solved the issue by adding a refund mechanism in the related handler.

Commit ID: 369bb8ba9926ad704e8aebd2692f03d9f343021e

The improper definition of an upper bound on fees in this system could potentially result in excessive fees and harm the user experience. This is because the upper bound is not adequately constrained, and it is possible for fees to be set at levels that are much higher than expected or reasonable. This can create a financial burden for users and discourage the use of the system. In addition, excessive fees can also lead to centralization and concentration of power in the hands of those who can afford to pay high fees, which can create additional security and integrity risks for the system. It is important for the system to properly define the upper bound on fees to ensure that fees are not set at excessive levels and to protect the user experience and the security of the system.

Code Location

Bridge.sol#L298

    /**
        @notice Changes deposit fee.
        @notice Only callable by admin.
        @param newFee Value {_fee} will be updated to.
     */
    function adminChangeFee(uint newFee) external onlyAdmin {
        require(_fee != newFee, "Current fee is equal to new fee");
        _fee = newFee;
    }

contract Tester is DSTest {

     address public user1 = address(0xB0B);
     Bridge internal bridge;
     NativeHandler internal nativeHandler;
     CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
   function setUp() public {
        address [] memory path = new address[](2);
        path[0] = address(this);
        path[1] = address(this);
        bridge = new Bridge(1,path,5,100,100);

        bytes32 [] memory pathByteGG = new bytes32[](2);
        pathByteGG[0] = keccak256(abi.encodePacked("User1"));
        pathByteGG[1] = keccak256(abi.encodePacked("User2"));

        nativeHandler = new NativeHandler(address(bridge),pathByteGG,path,path);

        bytes32 tokenResource = keccak256(abi.encodePacked("NATIVE"));
        bridge.adminSetResource(address(nativeHandler),tokenResource,0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        cheats.deal(user1,1_000_000 ether);
        cheats.deal(address(this), 1_000_000 ether);
   }

   function testFee() public {
        bridge.adminChangeFee(1000000000000000000000000);
   }

}

SOLVED: The Chiliz team solved the issue by adding an upper limit to the fee.

Commit ID: d715e18b47b17b63d3b0fdeb51dade1b18e535ee

The transfer function is not recommended for sending native token due to its, 2300 gas unit limit. Instead, call can be used to circumvent the gas limit.

Code Location

Bridge.sol#L308

    function deposit(uint8 destinationChainID, bytes32 resourceID, bytes calldata data) external payable whenNotPaused {
        require(msg.value >= _fee, "invalid value");

        address handler = _resourceIDToHandlerAddress[resourceID];
        require(handler != address(0), "resourceID not mapped to handler");

        uint64 depositNonce = ++_depositCounts[destinationChainID];
        _depositRecords[depositNonce][destinationChainID] = data;

        IDepositExecute depositHandler = IDepositExecute(handler);
        if (msg.value > 0) {
            uint256 valueToSend = msg.value - _fee;
            if (valueToSend > 0) {
                payable(handler).transfer(valueToSend);    
            }
        }
        depositHandler.deposit(resourceID, destinationChainID, depositNonce, msg.sender, data);

        emit Deposit(destinationChainID, resourceID, depositNonce);
    }

SOLVED: The Chiliz team solved the issue by changing the call transfer function.

Commit ID: 0976828419dbaa9feabfa017a1032c4203abf6da

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against re-entrancy attacks.

Code Location

Bridge.sol#L308

    function deposit(uint8 destinationChainID, bytes32 resourceID, bytes calldata data) external payable whenNotPaused {
        require(msg.value >= _fee, "invalid value");

        address handler = _resourceIDToHandlerAddress[resourceID];
        require(handler != address(0), "resourceID not mapped to handler");

        uint64 depositNonce = ++_depositCounts[destinationChainID];
        _depositRecords[depositNonce][destinationChainID] = data;

        IDepositExecute depositHandler = IDepositExecute(handler);
        if (msg.value > 0) {
            uint256 valueToSend = msg.value - _fee;
            if (valueToSend > 0) {
                payable(handler).transfer(valueToSend);    
            }
        }
        depositHandler.deposit(resourceID, destinationChainID, depositNonce, msg.sender, data);

        emit Deposit(destinationChainID, resourceID, depositNonce);
    }

NativeHandler.sol#L91

    function deposit(
        bytes32 resourceID,
        uint8   destinationChainID,
        uint64  depositNonce,
        address depositer,
        bytes   calldata data
    ) external override onlyBridge {
        bytes   memory recipientAddress;
        uint256        amount;
        uint256        lenRecipientAddress;

        assembly {
            amount := calldataload(0xC4)
            recipientAddress := mload(0x40)
            lenRecipientAddress := calldataload(0xE4)
            mstore(0x40, add(0x20, add(recipientAddress, lenRecipientAddress)))

            calldatacopy(
                recipientAddress, // copy to destinationRecipientAddress
                0xE4, // copy from calldata @ 0x104
                sub(calldatasize(), 0xE) // copy size (calldatasize - 0x104)
            )
        }

        address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
        require(_contractWhitelist[tokenAddress], "provided tokenAddress is not whitelisted");

        uint256 bridgeFee = IBridge(_bridgeAddress)._fee();
        uint256 amountMinusFee = amount - bridgeFee;
        require(amountMinusFee > 0, "Invalid amount");

        _depositRecords[destinationChainID][depositNonce] = DepositRecord(
            tokenAddress,
            uint8(lenRecipientAddress),
            destinationChainID,
            resourceID,
            recipientAddress,
            depositer,
            amountMinusFee
        );
    }

SOLVED: The Chiliz team solved the issue by adding a re-entrancy guard on the related functions.

Commit ID: 8d2c1cfb973eae4935ce167c8ae304ddcb9f843b

In the contract, NatSpec is incompatible with the implementation, which could lead to confusion and misunderstandings among users and developers. This is because NatSpec is a standard for documenting and annotating smart contracts, and it is designed to provide clear and concise information about the functions and behavior of a contract. However, if NatSpec is incompatible with the implementation of the contract, this information may not accurately reflect the actual behavior of the contract, and users and developers may not be able to fully understand and use the contract. It is important for the system to ensure that NatSpec is compatible with the implementation of the contract, to provide clear and accurate information to users and developers and to improve the usability and reliability of the contract.

Code Location

NativeHandler.sol#L89

    /*
        marked true in {_burnList}, deposited tokens will be burned, if not, they will be locked.
     */

SOLVED: The Chiliz team solved the issue by updating the natspec in the functions.

Commit ID: a2814edaea8eaf7c6df0a176846645834b9a8e3e

In this system, the Solidity code does not include a check to prevent sending transactions to the zero address. The zero address is a special address that is reserved in Ethereum, and it represents the absence of an address. Sending a transaction to the zero address is equivalent to sending it to nowhere, and it will be lost and not executed. However, if the Solidity code does not check for the zero address, it is possible for a malicious actor to exploit this and send transactions to the zero address, potentially resulting in the loss of funds. It is important for the system to include a zero address check in the Solidity code, to prevent transactions from being sent to the zero address and to protect against potential loss of funds.

Code Location

NativeHandler.sol#L104

    function deposit(
        bytes32 resourceID,
        uint8   destinationChainID,
        uint64  depositNonce,
        address depositer,
        bytes   calldata data
    ) external override onlyBridge {
        bytes   memory recipientAddress;
        uint256        amount;
        uint256        lenRecipientAddress;

        assembly {
            amount := calldataload(0xC4)
            recipientAddress := mload(0x40)
            lenRecipientAddress := calldataload(0xE4)
            mstore(0x40, add(0x20, add(recipientAddress, lenRecipientAddress)))

            calldatacopy(
                recipientAddress, // copy to destinationRecipientAddress
                0xE4, // copy from calldata @ 0x104
                sub(calldatasize(), 0xE) // copy size (calldatasize - 0x104)
            )
        }

        address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
        require(_contractWhitelist[tokenAddress], "provided tokenAddress is not whitelisted");

        uint256 bridgeFee = IBridge(_bridgeAddress)._fee();
        uint256 amountMinusFee = amount - bridgeFee;
        require(amountMinusFee > 0, "Invalid amount");

        _depositRecords[destinationChainID][depositNonce] = DepositRecord(
            tokenAddress,
            uint8(lenRecipientAddress),
            destinationChainID,
            resourceID,
            recipientAddress,
            depositer,
            amountMinusFee
        );
    }

SOLVED: The Chiliz team solved the issue by adding receiver address check.

Commit ID: bf171d4028b5af946c091baa77a413b3927a44bc

In the bridge, the NativeHandler contract includes a tokenAddress function parameter that is not used by the function itself. This redundant parameter could potentially cause confusion and misunderstandings, as it may not be clear to users and developers why the parameter is included or what it is used for. In addition, the presence of this redundant parameter could also increase the complexity of the code and make it more difficult to maintain and update. It is important for the system to avoid using redundant function parameters, to improve the clarity and usability of the code and to reduce the complexity and maintenance costs of the system.

Code Location

NativeHandler.sol#L180

    /**
        @notice Used to manually release native tokens from the handler.
        @param tokenAddress Address of token contract to release.
        @param recipient Address to release tokens to.
        @param amount The amount of native tokens to release.
     */
    function withdraw(address tokenAddress, address recipient, uint amount) external override onlyBridge {
        payable(recipient).transfer(amount);
    }

SOLVED: The Chiliz team solved the issue by changing the logic in the related function.

Commit ID: 3660ae57c26131770796467d4c123b2274cf6159

The presence of a redundant array definition in the constructor could potentially lead to confusion and misunderstanding, and could also lead to unnecessary code complexity and maintenance costs.

Code Location

NativeHandler.sol#L45

    constructor(
        address          bridgeAddress,
        bytes32[] memory initialResourceIDs,
        address[] memory initialContractAddresses,
        address[] memory burnableContractAddresses
    ) public {
        require(initialResourceIDs.length == initialContractAddresses.length,
            "initialResourceIDs and initialContractAddresses len mismatch");

        _bridgeAddress = bridgeAddress;

        for (uint256 i = 0; i < initialResourceIDs.length; i++) {
            _setResource(initialResourceIDs[i], initialContractAddresses[i]);
        }

        for (uint256 i = 0; i < burnableContractAddresses.length; i++) {
            _setBurnable(burnableContractAddresses[i]);
        }
    }

SOLVED: The Chiliz team solved the issue by deleting the redundant parameter in the constructor.

Commit ID: 568ac8b609f9945bbbafa454afe394fdd8270458

From Pragma 0.8.0, ABI coder v2 is activated by default. The pragma abicoder v2 can be deleted from the repository. That will provide gas optimization.

Code Location

Bridge.sol#L2

pragma solidity 0.6.4;
pragma experimental ABIEncoderV2;

import "@openzeppelin/contracts/access/AccessControl.sol";
import "./utils/Pausable.sol";
import "./utils/SafeMath.sol";
import "./interfaces/IDepositExecute.sol";
import "./interfaces/IBridge.sol";
import "./interfaces/IERCHandler.sol";
import "./interfaces/IGenericHandler.sol";

/**
    @title Facilitates deposits, creation and votiing of deposit proposals, and deposit executions.
    @author ChainSafe Systems.
 */
contract Bridge is Pausable, AccessControl, SafeMath 

NativeHandler.sol#L2

pragma solidity 0.6.4;
pragma experimental ABIEncoderV2;

import "../interfaces/IDepositExecute.sol";
import "../interfaces/IBridge.sol";
import "./HandlerHelpers.sol";
import "../ERC20Safe.sol";
import "@openzeppelin/contracts/presets/ERC20PresetMinterPauser.sol";

contract NativeHandler is IDepositExecute, HandlerHelpers, ERC20Safe {}

SOLVED: The Chiliz team solved the issue by updating pragma.

Commit ID: 6830bf7e74f4a2bb7675c768f24a8a2f35889df5

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks for free.

The advantages of versions =0.8.*= over =<0.8.0= are:

• Safemath by default from 0.8.0 (can be more gas efficient than some library based safemath).
• Low level inliner from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases, used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

SOLVED: The Chiliz team solved the issue by updating pragma.

Commit ID: 6830bf7e74f4a2bb7675c768f24a8a2f35889df5

The check != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled. While it may seem that > 0 is cheaper than !=0, this is only true without the optimizer enabled and outside a require statement. If the optimizer is enabled at 10k, and it is in a require statement, that would be more gas efficient.

Code Location

Bridge.sol#L302-L304

        IDepositExecute depositHandler = IDepositExecute(handler);
        if (msg.value > 0) {
            uint256 valueToSend = msg.value - _fee;
            if (valueToSend > 0) {
                payable(handler).transfer(valueToSend);    
            }
        }

SOLVED: The Chiliz team solved the issue by deleting the related code section in the contract.

Commit ID: 6830bf7e74f4a2bb7675c768f24a8a2f35889df5

There is a function declared as public that is never called internally within the contract. It is best practice to mark such functions as external instead, as this saves gas (especially in the case where the function takes arguments, as external functions can read arguments directly from calldata instead of having to allocate memory).

Code Location

Bridge.sol#L384

    function cancelProposal(uint8 chainID, uint64 depositNonce, bytes32 dataHash) public onlyAdminOrRelayer 

SOLVED: The Chiliz team solved the issue by changing the visibility of the function.

Commit ID: f574625d79d4701d2a89acfd9a5b139c352e9a91

In a Solidity contract, the developer has included multiple instances of the SafeMath library functions, even though only one instance is needed. Including redundant instances of the SafeMath library can lead to an increase in the size of the contract, which can negatively impact the contract's performance and gas usage.

Code Location

Bridge.sol#L151

    function add(uint256 a, uint256 b) internal pure returns (uint256) {
        uint256 c = a + b;
        require(c >= a, "SafeMath: addition overflow");

        return c;
    }

    /**
     * @dev Returns the subtraction of two unsigned integers, reverting on
     * overflow (when the result is negative).
     *
     * Counterpart to Solidity's `-` operator.
     *
     * Requirements:
     * - Subtraction cannot overflow.
     */
    function sub(uint256 a, uint256 b) internal pure returns (uint256) {
        return sub(a, b, "SafeMath: subtraction overflow");
    }

    /**
     * @dev Returns the subtraction of two unsigned integers, reverting with custom message on
     * overflow (when the result is negative).
     *
     * Counterpart to Solidity's `-` operator.
     *
     * Requirements:
     * - Subtraction cannot overflow.
     */
    function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
        require(b <= a, errorMessage);
        uint256 c = a - b;

        return c;
    }

SOLVED: The Chiliz team solved the issue by upgrading pragma 0.8.17 in contracts. Also, the SafeMath libraries are deleted from the contracts.

Commit ID: add9f81660e4ab58778f4706e00fc4ac4234a153