DestinationBridge contract: transactions can be executed even if the contract is paused

[code423n4]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L197-L203
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L156-L167
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L337-L353

Vulnerability details

Impact

• The purpose of adding pausability feature in a contract is to prevent the execution of its functions in certain cases; which are mostly emergency cases (when calls to some contracts/tokens start to act maliciously).

• The DestinationBridge contract implements this feature; where transactions initiation (calling execute) is disabled when the contract is paused .

• But even if the contract is paused; approvers can still approve transactions (by calling approve that doesn't check if the contract is paused) and eventually leading these transactions to be executed if they met the approvals threshold.

• This will break the main purpose of adding the pausability feature; which is protecting the contract in the cases of emergancies; such when the bridged token contract starts to act in a malicious way that will lead to harming the destination bridge.

Proof of Concept

• Code:
  approve function

  File:2023-09-ondo/contracts/bridge/DestinationBridge.sol
  Line 197-203:
    function approve(bytes32 txnHash) external {
      if (!approvers[msg.sender]) {
        revert NotApprover();
      }
      _approve(txnHash);
      _mintIfThresholdMet(txnHash);
    }

  _approve function

  File:2023-09-ondo/contracts/bridge/DestinationBridge.sol
  Line 156-167:
    function _approve(bytes32 txnHash) internal {
      // Check that the approver has not already approved
      TxnThreshold storage t = txnToThresholdSet[txnHash];
      if (t.approvers.length > 0) {
        for (uint256 i = 0; i < t.approvers.length; ++i) {
          if (t.approvers[i] == msg.sender) {
            revert AlreadyApproved();
          }
        }
      }
      t.approvers.push(msg.sender);
    }

  _mintIfThresholdMet function

  File:2023-09-ondo/contracts/bridge/DestinationBridge.sol
  Line 156-167:
    function _mintIfThresholdMet(bytes32 txnHash) internal {
      bool thresholdMet = _checkThresholdMet(txnHash);
      Transaction memory txn = txnHashToTransaction[txnHash];
      if (thresholdMet) {
        _checkAndUpdateInstantMintLimit(txn.amount);
        if (!ALLOWLIST.isAllowed(txn.sender)) {
          ALLOWLIST.setAccountStatus(
            txn.sender,
            ALLOWLIST.getValidTermIndexes()[0],
            true
          );
        }
        TOKEN.mint(txn.sender, txn.amount);
        delete txnHashToTransaction[txnHash];
        emit BridgeCompleted(txn.sender, txn.amount);
      }
    }

• Foundry PoC:

1. test_Transaction_Executed_When_Contract_Is_Paused() test is added to forge-tests/bridges/DestinationBridge.t.sol file; with the following scenario:

   • a relayer initiates a transaction execution when the contract is not paused.
   • then the DestinationBridge contract owner pauses the contract.
   • approvers approve the tranaction until it gets executed (bridged tokens minted to the user).

   function test_Transaction_Executed_When_Contract_Is_Paused() public {
   string memory srcChain = "arbitrum";
   string memory srcAddress = "0xce16F69375520ab01377ce7B88f5BA8C48F8D666";
   address srcSender = alice;
   uint256 amt = 100e18;
   address nonApprover = address(500);
   
   //2. a transaction of 100e18 amount is initiated and then executed by the nonApprover as it need one approver only:
   assertEq(destinationBridge.approvers(nonApprover), false);
   assertEq(usdy.balanceOf(srcSender), 0); //the srcSender doesnt have any usdc on the destination chain befor transaction execution
   
   bytes32 cmdId = bytes32(
     0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e0
   );
   uint256 nonce = 1;
   bytes memory payload = abi.encode(VERSION, srcSender, amt, nonce);
   approve_message(
     cmdId,
     srcChain,
     srcAddress,
     address(destinationBridge),
     keccak256(payload)
   );
   destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
   
   //2. Then the admin pauses the destination bridge contract :
   assertEq(destinationBridge.paused(), false);
   vm.prank(guardian);
   destinationBridge.pause();
   assertEq(destinationBridge.paused(), true);
   
   //3. the transaction needs two approvals; the first approval is given by the relayer who initiated the transaction execution,and the second approval is given by the approved ondo signer:
   vm.prank(ondoSigner0);
   destinationBridge.approve(keccak256(payload));
   assertEq(destinationBridge.getNumApproved(keccak256(payload)), 2);
   
   //4. the transaction executed :
   assertEq(usdy.balanceOf(srcSender), amt);
   }

2. Test result:

   $ forge test --fork-url $(grep -w MAINNET_RPC_URL .env | cut -d '=' -f2) --fork-block-number $(grep -w FORK_FROM_BLOCK_NUMBER_MAINNET .env | cut -d '=' -f2) --nmc Test_Prod --mc '(Test_DestinationBridge)'
   [PASS] test_Transaction_Executed_When_Contract_Is_Paused() (gas: 306194)

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Update approve function to be called only when the contract is not paused:

- function approve(bytes32 txnHash) external {
+ function approve(bytes32 txnHash) external whenNotPaused {

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as duplicate of #529

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-c

[c4-judge]

kirk-baird marked the issue as grade-b