[Bridge] [Solidity] Use `require` for input validation rather than `if-revert` pattern

[franck44]

Problem

Using require to validate inputs

In our (bridge) Solidity contracts, we are using the pattern if (!cond) revert Error to validate inputs. This is not the best practice and there were several instances of bugs in the past where the contract had if !C someCode; revert(), ommitting the {} around the if-body, which can have unintended consequences. You can read more about the risks (and a bug in APPLE-SSL) here.

[!WARNING] In short, if written by hand, there is no guarantee that the if-revert pattern is used properly.

Using require(cond, error) has several advantages:

• require clearly identify the condition cond that should be checked rather than the negation of the condition that appears in the equivalent if-revert pattern;
• require(cond, error) is automatically translated (by the Solidity compiler) in if (!cond) revert error thereby enforcing that the pattern if-revert is used correctly.

[!IMPORTANT] The use of require is recommended to validate inputs and has better readability and security.

CustomError to minimise gas cost

There is a caveat though: until recently, require(cond, error) only supported error of type int or strings, and these two types are more expensive gas-wise than custom errors. This is why developers would deliberately use if (!C) revert CustomError (instead of the require) in the code to optimise gas costs, while compromising security.

As of version 0.8.27 (release notes) , the Solidity compiler supports require(conditional, customError).

[!TIP] We can now use a safe and gas-efficient pattern require(cond, customError).

Proposed changes

There are two Solidity contracts that are currently using the unsafe and hard-to-read if-revert pattern to validate inputs:

• AtomicBridgeInitiatorMOVE.sol
• AtomicBridgeCounterpartyMOVE.sol

The proposal is to use require for input validation.

For example, the validation of inputs amount > 0 in the function initiateBridgeTransfer:

function initiateBridgeTransfer(uint256 moveAmount, bytes32 recipient, bytes32 hashLock)
        external
        returns (bytes32 bridgeTransferId)
    {
        address originator = msg.sender;

        // Ensure there is a valid amount
        if (moveAmount == 0) {
            revert ZeroAmount();
        }

        // Transfer the MOVE tokens from the user to the contract
        if (!moveToken.transferFrom(originator, address(this), moveAmount)) {
            revert MOVETransferFailed();
        }

       ...
    }

would be re-written into (note that the revert related to the transfer of Move tokens is not an input validation, but a more complex error and best practice is to use an if ... revert in that case):

function initiateBridgeTransfer(uint256 moveAmount, bytes32 recipient, bytes32 hashLock)
        external
        returns (bytes32 bridgeTransferId)
    {
        address originator = msg.sender;

        // Ensure there is a valid amount
        require(moveAmount > 0, ZeroAmount());
        // if (moveAmount == 0) {
        //    revert ZeroAmount();
        // }

        // Transfer the MOVE tokens from the user to the contract
        if (!moveToken.transferFrom(originator, address(this), moveAmount)) {
            revert MOVETransferFailed();
        }

       ...
    }

Implementation

To implement the changes we have to:

• re-write the input validation conditions into require
• use Solidity compiler version >= 0.8.27 (which is recommended anyway as recent versions fix compiler bugs and improve performance)
• run the test suite (if any test exists)

[!IMPORTANT] There does not seem to be any test in AtomicBridgeInitiatorMOVE.t.sol to check that the functions revert under certain conditions. If is absolutely necessary to add some tests to ensure that we don't disable the checks (e.g. amount > 0) later and introduce some bugs.

Validation

Here are the steps to validate the changes:

1. we need to add more tests to AtomicBridgeInitiatorMOVE.t.sol. There should be at least one test for each input validation and expected failure (revert).
   2. we can check that the tests pass and that input are properly validated and the functions revert as expected.

[andygolay]

LGTM, thanks @franck44.

What's the process for "approving" issues? I could implement this as a PR but want to avoid duplicating other people's work and PR race conditions so to speak, as well.

[andygolay]

I created and tested #852 to address this, tagged you for review @franck44