Messages can not be sent when amb is down

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x0e94ba4b8f9ec8da612c89ef254eeb51de62079f1110b7ba3ee39d0dc10094ca Severity: medium

Description: Description\ AMBBridgeGateway.sol connects to the corresponding Arbitrary Message Bridge in this chain, to the CrossChainProofOfHumanity Proxy and to the foreign gateway contract (deployed on the side chain).

This issue is related to sendMessage() function which sends message from CrosschainProofOfHumanity on this chain to foreign gateway which will receive and redirect to CrosschainProofOfHumanity on side chain.

    function sendMessage(bytes memory _data) external override {
        require(msg.sender == homeProxy, "!homeProxy");
        amb.requireToPassMessage(foreignGateway, abi.encodeCall(this.receiveMessage, (_data)), amb.maxGasPerTx());
    }

The issue is that, when the AMB is down the message can not be sent. According to Gnosis bridge AMB documentation, If the hardfork of AMB is planned for downtime (not relaying message) to ensure security of the bridge then the AMB would be set to 0 by AMB contract owner by calling below function from AMB contract.

    function setMaxGasPerTx(uint256 _maxGasPerTx) external onlyOwner {
        uintStorage[MAX_GAS_PER_TX] = _maxGasPerTx;
    }

Currently Gnosis has set maxGasPerTx is set to 4000000 on Ethereum mainnet and 2000000 on Gnosis Chain.

By setting maxGasPerTx to 0, the condition in _sendMessage() will not pass, meaning, the bridge is down/stopped. since _sendMessage() is used as an internal function in amb.requireToPassMessage() function so the sendMessage() will not be able to pass.

Impact\ Message can not be passed to gatways if amb is down or stopped. This would break contracts core functionality.

Recommendations\ Consider checking amb is down or stopped in sendMessage() function.

One way is to check, amb.maxGasPerTx() != 0 since it would only be set to 0 when amb is stopped or down.

Consider following changes:

    function sendMessage(bytes memory _data) external override {
        require(msg.sender == homeProxy, "!homeProxy");
+      require(amb.maxGasPerTx() != 0, "amb is stopped/down");
        amb.requireToPassMessage(foreignGateway, abi.encodeCall(this.receiveMessage, (_data)), amb.maxGasPerTx());
    }

[clesaege]

This isn't required as per the documentation if the maxGasPerTx is set to 0, this function require will fail, which will make the sendMessage to also fail. Once the AMB is back up, it will be possible call sendMessage again.

[0xRizwan]

@clesaege Agreed but it won't explicitely revert with amb is down/stopped. The end user transaction would be reverted but he wont be able to know the transaction revert reason. I would still suggest to add the recommendation validation check in sendMessage()

[clesaege]

It would explicitly revert due to this line. Require reasons is a new solidity feature and not required (actually it does increase contract size and in your example it would also increase contract complexity), I believe the revert is already handled well at the MessageDelivery level and handling it at two different ways would unnecessarily increase smart contract complexity.

You may have a different opinion, but per contest rules are excluded: Issues about code/project quality which do not lead to exploitable vulnerabilities.