[Bridge] [Solidity] Pool balance used to guard `withdrawMOVE` function

[franck44]

Problem

In the AtomicBridgeInitiatorMOVE.sol contract, the poolBalance variable that is supposed to track the current amount of locked tokens in the bridge is used in withdrawMOVEand can prevent a withdrawal (moveToken.transfer).

function withdrawMOVE(address recipient, uint256 amount) external {
        if (msg.sender != counterpartyAddress) revert Unauthorized();
        if (poolBalance < amount) revert InsufficientMOVEBalance();
        poolBalance -= amount;
        if (!moveToken.transfer(recipient, amount)) revert MOVETransferFailed();
    }

[!WARNING]

1. We have no guarantee that the poolBalance tracks the number of tokens locked in the bridge via bridge transfers. The moveToken is an ERC-20 token, and there could be transfers to and from the account that do not go through the bridge. Examples are if the bridge is compromised we may have to move MOVE tokens in or out manually.
2. as a result, the test poolBalance >= amount does not guarantee that the transfer (next line) will be successful.
3. the fact that poolBalance and the actual balance in the bridge moveToken.Balance may be different could create some issues: assume the bridge is compromised, the poolBalance is zero, and we have to manually refill with say $1B MOVE tokens. Because amount is zero, users won't be able to withdraw (bridge back from L2) even if there is $1B MOVE in the bridge, which is probably not what is expected as we just refilled the bridge.
4. another issue is that the withdraw function is in the AtomicBridgeInitiatorMOVE.sol contract but is not used in this contract, only in the AtomicBridgeCounterpartyMOVE.sol.

Here is the completeBridgeTransfer function in AtomicBridgeCounterpartyMOVE.sol:

 function completeBridgeTransfer(bytes32 bridgeTransferId, bytes32 preImage) external {
        BridgeTransferDetails storage details = bridgeTransfers[bridgeTransferId];
        if (details.state != MessageState.PENDING) revert BridgeTransferStateNotPending();
        bytes32 computedHash = keccak256(abi.encodePacked(preImage));
        if (computedHash != details.hashLock) revert InvalidSecret();
        if (block.timestamp > details.timeLock) revert TimeLockExpired();

        details.state = MessageState.COMPLETED;

        atomicBridgeInitiatorMOVE.withdrawMOVE(details.recipient, details.amount);

        emit BridgeTransferCompleted(bridgeTransferId, preImage);
    }

This results in at least two issues:

• [Issue 1] the guard poolBalance >= amount may prevent use from fixing a compromised bridge
• [Issue 2] the AtomicBridgeCounterpartyMOVE.sol contract uses a contract call to unlock the tokens: a) this is not gas-efficient and b) creates a dependency between the two contracts; if withdraw in AtomicBridgeInitiatorMOVE.sol is modified, this impacts the behaviour of the counter party.

Possible changes

One thing that should probably be done is: remove the line if (poolBalance < amount) revert InsufficientMOVEBalance(); in the AtomicBridgeInitiatorMOVE.sol contract as it does not serve any purpose and can be dangerous (Issue 1 above).

[!NOTE] We can keep the poolBalance variable to track what has been locked and unlocked in the bridge (which may be different to the balance of the contract). Option 2 below proposes an alternative solution.

Once this is done we have two options to address the cross-contract call:

1. keep the poolBalanceand merge the code of the two contracts
2. split the poolBalance into poolLocked and poolUnlocked, with poolLocked (respectively poolUnlocked) tracking what has been locked in the bridge (via initiateBridgeTransfer) and poolUnlocked tracking what has been unlocked from the bridge (via completeBridgeTransfer in AtomicBridgeCounterpartyMOVE.sol. Concretely, we remove poolBalance and replace with poolLocked in AtomicBridgeInitiatorMOVE.sol and we add poolUnlocked in AtomicBridgeCounterpartyMOVE.sol.

Option 1 has one advantage: it is easy to implement, but a serious drawback as we may not be able to track the bridge poolBalance (or anything related to how much went in and out the bridge).

Option 2 above has two advantages:

1. we are able to track what goes in and out of the bridge, and the bridge poolBalance can be reconstructed from poolLocked - poolUnlocked
2. if keeps the two contracts Initiator (bridging L1 to L2) and the CounterParty (bridging L2 to L1) indepedent as we can move the withdraw function out of AtomicBridgeInitiatorMOVE.sol into in completeBridgeTransfer (or inline it) in AtomicBridgeCounterpartyMOVE.sol.

Implementation

One we have decided whether or not we want to fix issues 1 and 2, and which option is preferred, we can implement the changes. This requires:

• potentially adding two variables poolLocked and poolUnlocked in the two contracts
• removing the cross-contract call and migrating (or inlining) the withdraw function.

In practice, in AtomicBridgeInitiatorMOVE.sol, we rename poolBalance into poolLocked, and in AtomicBridgeCounterpartyMOVE.sol we add a variable poolUnlocked and modify the function completeBridgeTransfer to:

// Total MOVE token transferred out of the bridge on L1 (unlocked)
uint256 public poolUnlocked;

function completeBridgeTransfer(bytes32 bridgeTransferId, bytes32 preImage) external {
        // Only bridge transfer operator can perform the unlock
        requires(msg.sender == counterpartyAddress, Unauthorized());

        // Check bridge transfer details
        BridgeTransferDetails storage details = bridgeTransfers[bridgeTransferId];
        // Must not have been completed yet
        requires(details.state == MessageState.PENDING, BridgeTransferStateNotPending();
        // Secret is known and deadline (timeLock) not elapsed
        bytes32 computedHash = keccak256(abi.encodePacked(preImage));
        requires(computedHash == details.hashLock, InvalidSecret());
        requires(block.timestamp <= details.timeLock, TimeLockExpired());

         // Mark transfer as complete
        details.state = MessageState.COMPLETED;
        // Update amount of unlock
        poolUnlocked += amount;
        // Try to transfer to recipient or revert
        if (!moveToken.transfer(recipient, amount)) revert MOVETransferFailed();

        emit BridgeTransferCompleted(bridgeTransferId, preImage);
    }

Validation

We need to add some unit tests to test that:

• reverting conditions are correctly triggered (the calls revert when we expect them to)
• the poolLocked and poolUnlocked variables are updated properly (and this for each function in each contract).

[0xPrimata]

On L1: poolUnlocked is the difference between 10B (max supply) and current balance and poolLocked is the balance of bridgeInitiator Contract On L2: poolUnlocked is the current supply and poolLocked is the difference between 10B (max supply) and the current supply. Don't think adding more params helps us, we could these constants for extra care.

One issue I can foresee is if there's an exploit, using poolUnlocked and poolLocked will lead to bugs as invalid minting or removal of tokens would lead to an unbalance between the tokens.

note: poolBalance has already shown to bug and ERC20() _transfer already handles the logic it was meant to serve. removal of poolBalance has been merged into Solidity Bridge Deployment Scripts

[franck44]

One issue I can foresee is if there's an exploit, using poolUnlocked and poolLocked will lead to bugs as invalid minting or removal of tokens would lead to an unbalance between the tokens.

How can variables that are just updated but do not change the control flow create bugs? The poolLocked and poolUnlocked are not related to minting (this is in the ERC-20). This comment does not seem relevant here.

note: poolBalance has already shown to bug and ERC20() _transfer already handles the logic it was meant to serve. > removal of poolBalance has been merged into https://github.com/movementlabsxyz/movement/pull/681

How can a variable in a bridge contract "bug" another contract (an ERC-20)?

[0xPrimata]

it bugs, it doesn't bug another contract, it's buggy by itself because we are not implementing a full logic and the full logic is simply implemented by ERC20. They are attempting to perform the same logic, but currently do not do so. It's redudant logic.