ChainBridge

Description

Relayers can see any incoming transaction from other actors in the system. Especially important for the vulnerability described below, is the case where a non-relayer actor deposits tokens on blockchain A to transfer them to blockchain B (EVM-based).

In addition to the above, another essential condition that enables the exploitation of the vulnerability presented below, is the fact that relayers can all call the voteProposal function with defined depositNonce and dataHash , as noticeable by the line depicted below.

contracts/Bridge.sol:L294

function voteProposal(uint8 chainID, uint256 depositNonce, bytes32 dataHash) public onlyRelayers whenNotPaused {

As we can observe in the logic of function mentioned above, if a proposal has already been submitted a relayer will vote for it, if not, a new proposal is created.

The exploitable issue arises from the fact that there is no verification that dataHash is the same as in an existing proposal. Effectively allowing any relayer to be the first one to create the proposal (possibly even doing it beforehand) and “deceiving” every other relayer into automatically voting for it.

This means that any relayer can replace the intended “benign” proposal with a malicious proposal that withdraws all the tokens from the smart contract.

Substrate note: datahash is already a part of the key in the Substrate code. Therefore, this specific issue is not present in Substrate-based chains.

Recommendation

Ensure that the dataHash of the proposal matches the dataHash parameter that is passed in the voteProposal function.

Description

ERC20 token transfers are used in multiple functions of the ERC20Safe contract:

contracts/ERC20Safe.sol:L29-L34

function fundERC20(address tokenAddress, address owner, uint256 amount) public {
    IERC20 erc20 = IERC20(tokenAddress);
    erc20.transferFrom(owner, address(this), amount);

    _balances[tokenAddress] = _balances[tokenAddress].add(amount);
}

contracts/ERC20Safe.sol:L44-L49

function lockERC20(address tokenAddress, address owner, address recipient, uint256 amount) internal {
    IERC20 erc20 = IERC20(tokenAddress);
    erc20.transferFrom(owner, recipient, amount);

    _balances[tokenAddress] = _balances[tokenAddress].add(amount);
}

contracts/ERC20Safe.sol:L58-L63

function releaseERC20(address tokenAddress, address recipient, uint256 amount) internal {
    IERC20 erc20 = IERC20(tokenAddress);
    erc20.transfer(recipient, amount);

    _balances[tokenAddress] = _balances[tokenAddress].sub(amount);
}

These transfer and transferFrom calls may just return False instead of canceling the whole current transaction.

Substrate note: This part of the logic is absent in the substrate library and should still be written in the actual implementation.

Recommendation

Properly process ERC20 transfers.

Description

When relayers have already voted for a proposal, any relayer can execute it by calling executeProposal.

contracts/Bridge.sol:L336-L349

function executeProposal(uint8 chainID, uint256 depositNonce, address handler, bytes memory data) public onlyRelayers whenNotPaused {
    Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];

    require(proposal._status != ProposalStatus.Inactive, "proposal is not active");
    require(proposal._status == ProposalStatus.Passed, "proposal was not passed or has already been transferred");
    require(keccak256(abi.encodePacked(handler, data)) == proposal._dataHash,
        "provided data does not match proposal's data hash");

    IDepositExecute depositHandler = IDepositExecute(handler);
    depositHandler.executeDeposit(data);

    proposal._status = ProposalStatus.Transferred;
    emit ProposalExecuted(chainID, _chainID, depositNonce);
}

Meaning that, upon a token transfer, which happens in the following line:

depositHandler.executeDeposit(data);

it is possible to exploit a reentrancy opportunity in permitting tokens (e.g., some implementations of the ERC777 standard), and call executeProposal again.

The exploitation presented above works because the status of the proposal is changed after the transfer:

proposal._status = ProposalStatus.Transferred;

Substrate note: This part of the logic is absent in the Substrate library and should still be written in the actual implementation. When it is implemented, some version of “reentrancy” can, potentially, be an issue.

Recommendation

The two possible solutions for the vulnerability presented are:

1. proposal._status = ProposalStatus.Transferred; should be called before the token transfer.
2. Add reentrancy protection.

Description

When the deposit function of the generic handler is called, the GenericHandler contract should call a specific method of a specific contract, that is bound to resourceID.

contracts/handlers/GenericHandler.sol:L177-L183

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

if (_contractAddressToDepositFunctionSignature[contractAddress] != bytes4(0)) {
    (bool success,) = contractAddress.call(metadata);
    require(success, "delegatecall to contractAddress failed");
}

The issue arises from the fact that the user can specify a different function in the metadata parameter, effectively calling this “malicious” function instead of the originally intended one.

Substrate note: This part of the logic is absent in the Substrate library and should still be written in the actual implementation.

Recommendation

Check if metadata contains the same function as _contractAddressToDepositFunctionSignature[contractAddress].

Description

As soon as a new block is included in a chain, events are processed and actions are executed. This creates the risk of processing some events that are part of a block reorg.

chains/ethereum/listener.go:L101-L105

// Sleep if the current block > latest
if currBlock.Number.Cmp(latestBlock) == -1 {
	time.Sleep(BlockRetryInterval)
	continue
}

Examples

Reorgs happen quite often on Ethereum. You can see the uncle rate in the link below.

https://etherscan.io/blocks_forked

Substrate note: Substrate chains might have different consensus algorithms in its configuration. Depending on the consensus mechanism there should be a different number of confirmation blocks (0 when an algorithm with instantaneous finality is used).

Recommendation

Enforce a delay of at least 10 blocks. This will reduce the number of processed uncles to a number close to zero.

Description

On the Substrate side, during voting periods, the following function is called to define whether the vote is completed or not:

chainbridge/src/lib.rs:L60-L70

fn try_to_complete(&mut self, threshold: u32, total: u32) -> bool {
    if self.votes_for.len() >= threshold as usize {
        self.status = ProposalStatus::Approved;
        true
    } else if self.votes_against.len() > (total - threshold) as usize {
        self.status = ProposalStatus::Rejected;
        true
    } else {
        false
    }
}

However, after the call is completed, the result of the vote is not properly checked. Only the fact that it has finished is taken into account.

While in most cases it is true that the result of the completed vote can be predicted, there are some edge-cases when this is not true. For example, when the total or threshold variables are updated. This may lead to executing proposals that should be canceled and vice versa.

Ethereum note: the Solidity implementation does not allow voting against a proposal, hence no cancel_execution is possible. So, when a vote is complete, the result can only be positive.

Recommendation

Check the result of the vote after it is completed.

Description

It is much harder to handle any error in Substrate than in Ethereum smart contracts. While on Ethereum one can use the revert opcode and be sure that all storage reverts to the previous state, the same is not true for Substrate. All storage changes that occur before an error is thrown remain intact.

The best practice in Substrate is to follow the “verify first, write last” principle. During execution, one should guarantee all the checks are using the ensure macro and that, upon starting to modify storage, no error can be thrown after.

While the outlined approach sounds clean, it can get very complicated when inter-pallet communication is involved. It is especially true when multiple different external pallets need to be called in one function. In such cases, it becomes extremely difficult to perform all the verifications before changing storage.

Examples

There are a few places where this problem may occur:

• transfer_fungible, transfer_nonfungible, transfer_generic functions of the Substrate library are called during the deposit from another pallet. They make use of the ensure macro, so they may potentially fail.

  chainbridge/src/lib.rs:L487-L506

  pub fn transfer_fungible(
      dest_id: ChainId,
      resource_id: ResourceId,
      to: Vec<u8>,
      amount: U256,
  ) -> DispatchResult {
      ensure!(
          Self::chain_whitelisted(dest_id),
          Error::<T>::ChainNotWhitelisted
      );
      let nonce = Self::bump_nonce(dest_id);
      Self::deposit_event(RawEvent::FungibleTransfer(
          dest_id,
          nonce,
          resource_id,
          amount,
          to,
      ));
      Ok(())
  }
  

  This means that these functions should be called either before any storage changes (outside of current pallet) or the same checks should be done in the outside pallet, before changing the state. Even though example pallets are out of scope, in this example, you can see that the actual transfer is happening before calling the transfer_fungible function:

  example-pallet/src/lib.rs:L72-L80

  pub fn transfer_native(origin, amount: BalanceOf<T>, recipient: Vec<u8>, dest_id: bridge::ChainId) -> DispatchResult {
      let source = ensure_signed(origin)?;
      ensure!(<bridge::Module<T>>::chain_whitelisted(dest_id), Error::<T>::InvalidTransfer);
      let bridge_id = <bridge::Module<T>>::account_id();
      T::Currency::transfer(&source, &bridge_id, amount.into(), AllowDeath)?;
   
      let resource_id = T::NativeTokenId::get();
      <bridge::Module<T>>::transfer_fungible(dest_id, resource_id, recipient, U256::from(amount.saturated_into()))
  }
  

  That may lead to tokens actually being transferred to the Bridge, but the event to transfer them outside is not emitted.

• When a proposal passes, its state is saved to storage before calling finalize_execution function.

chainbridge/src/lib.rs:L421-L429

let complete = votes.try_to_complete(<RelayerThreshold>::get(), <RelayerCount>::get());
<Votes<T>>::insert(src_id, (nonce, prop.clone()), votes.clone());
Self::deposit_event(RawEvent::VoteFor(src_id, nonce, who.clone()));

if complete {
    Self::finalize_execution(src_id, nonce, prop)
} else {
    Ok(())
}

inside of the finalize_execution function, an external pallet is called (whose implementation is still inexistent at the time of writing), which can also potentially fail. Moreover, even in the event the scenario described above materializes, the proposal will still be considered as executed.

Recommendation

Make sure no errors are being thrown after storage changes or build a mechanism that reverts all changes after an error is thrown.

Description

When an ERC 721 Deposit is handled, the recipient’s address is retrieved from the event.

chains/ethereum/events.go:L34-L39

record, err := l.erc721HandlerContract.GetDepositRecord(&bind.CallOpts{}, nonce.Big(), uint8(destId))
if err != nil {
	l.log.Error("Error Unpacking ERC20 Deposit Record", "err", err)
}

recipient := record.DestinationRecipientAddress[:record.LenDestinationRecipientAddress.Int64()]

The length of the address is handled in the ERC 721 handler.

contracts/handlers/ERC721Handler.sol:L117-L131

// Load length of recipient address from data + 96
lenDestinationRecipientAddress := mload(add(data, 0x60))
// Load free mem pointer for recipient
destinationRecipientAddress := mload(0x40)
// Store recipient address
mstore(0x40, add(0x20, add(destinationRecipientAddress, lenDestinationRecipientAddress)))

// func sig (4) + destinationChainId (padded to 32) + depositNonce (32) + depositor (32) +
// bytes lenght (32) + resourceId (32) + tokenId (32) + length (32) = 0xE4

calldatacopy(
    destinationRecipientAddress,    // copy to destinationRecipientAddress
    0xE4,                           // copy from calldata after destinationRecipientAddress length declaration @0xE4
    sub(calldatasize(), 0xE4)       // copy size (calldatasize - (0xE4 + 0x20))
)

However, token handlers can be swapped and the event handler should not rely on the correctness and safeness of the token handler.

The problem is that if the handler generates an event with a DestinationRecipientAddress shorter than LenDestinationRecipientAddress, the node will panic.

Recommendation

The node should be able to return an error, in case the decoding is faulty.

Such a case should be when LenDestinationRecipientAddress > len(DestinationRecipientAddress).

Description

Anyone can call the deposit function to transfer funds from a supported chain to any other.

contracts/Bridge.sol:L269-L279

function deposit (uint8 destinationChainID, address handler, bytes memory data) public payable whenNotPaused {
    require(msg.value == _fee, "Incorrect fee supplied");

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

    IDepositExecute depositHandler = IDepositExecute(handler);
    depositHandler.deposit(destinationChainID, depositNonce, msg.sender, data);

    emit Deposit(destinationChainID, handler, depositNonce);
}

The issue arises from the fact that the caller can pass any address as the handler. In addition, the Bridge contract will call this address, which might prove unsafe under certain conditions.

Substrate note: This part of the logic is absent in the Substrate library and should still be written in the actual implementation.

Recommendation

Only allow passing an authorized handler address as the parameter.

Description

In both the ERC20Safe and ERC721Safe contracts, the _balances field should represent the current balance of the contract. During minting, _balances is increased even though the recipient of the minting is not the contract itself but an external agent:

contracts/ERC20Safe.sol:L72-L77

function mintERC20(address tokenAddress, address recipient, uint256 amount) internal {
    ERC20PresetMinterPauser erc20 = ERC20PresetMinterPauser(tokenAddress);
    erc20.mint(recipient, amount);

    _balances[tokenAddress] = _balances[tokenAddress].add(amount);
}

contracts/ERC721Safe.sol:L73-L78

function mintERC721(address tokenAddress, address recipient, uint256 tokenID, bytes memory data) internal {
    ERC721MinterBurnerPauser erc721 = ERC721MinterBurnerPauser(tokenAddress);
    erc721.mint(recipient, tokenID, string(data));

    _balances[tokenAddress] = _balances[tokenAddress].add(1);
}

Substrate note: This part of the logic is absent in the substrate library and should still be written in the actual implementation.

Recommendation

Do not increase _balances during minting if the recipient is not the contract itself. Or, in alternative, use the balanceOf function of the token, instead.

Description

If the admin adds a relayer that was previously admitted, already:

contracts/Bridge.sol:L161-L165

function adminAddRelayer(address relayerAddress) public onlyAdmin {
    grantRole(RELAYER_ROLE, relayerAddress);
    emit RelayerAdded(relayerAddress);
    _totalRelayers++;
}

or removes a relayer that was already removed:

contracts/Bridge.sol:L173-L177

function adminRemoveRelayer(address relayerAddress) public onlyAdmin {
    revokeRole(RELAYER_ROLE, relayerAddress);
    emit RelayerRemoved(relayerAddress);
    _totalRelayers--;
}

_totalRelayers will still change by an absolute value of 1 even though the effective number of relayers is left unchanged. In addition, the variable can underflow/overflow.

Substrate note: the Substrate library is already implementing the proper checks when adding/removing relayers.

Recommendation

Make sure that admin is adding/removing non-existing/existing relayer.

Description

An event should never be sent to the router if it does not match any of the recognized handlers.

An error is logged, but the execution is not stopped.

chains/ethereum/listener.go:L141-L154

if addr == l.cfg.erc20HandlerContract {
	m = l.handleErc20DepositedEvent(destId, nonce)
} else if addr == l.cfg.erc721HandlerContract {
	m = l.handleErc721DepositedEvent(destId, nonce)
} else if addr == l.cfg.genericHandlerContract {
	m = l.handleGenericDepositedEvent(destId, nonce)
} else {
	l.log.Error("Event has unrecognized handler", "handler", addr)
}

err = l.router.Send(m)
if err != nil {
	l.log.Error("subscription error: failed to route message", "err", err)
}

Recommendation

Skip routing the event if the handler does not match the allowed ones.

Description

In Substrate, it’s only possible to calculate voting results when a new vote is submitted.

However, if total or threshold are updated, it should be possible to check the results of the current votings and call finalize_execution (or cancel_execution) without submitting a vote. This very closely resembles a similar problem presented in the Solidity implementation.

Even though it is possible to execute the proposal without making a vote, the proposal’s status should be ProposalStatus.Passed, which can only be assigned when making a vote.

Recommendation

Add a public function that checks if a vote can be completed and executed.

Description

Substrate does not have overflow/underflow protection by default. In order to avoid this class of bugs, it is recommended to use safe functions like checked_add() and checked_sub(). In the current codebase math operations are mostly either incrementing (+1) or making operations with values that shouldn’t overflow/underflow logically:

chainbridge/src/lib.rs:L64

} else if self.votes_against.len() > (total - threshold) as usize {

Even though, in this specific instance, this does not present a big risk, if the threshold is bigger than total, an underflow will occur.

Ethereum note: SafeMath library is already used Ethereum contracts for most of the math operations.

Recommendation

Use safe functions like checked_add() and checked_sub() for math operations.

Description

contracts/handlers/HandlerHelpers.sol:L42-L45

bytes32 currentResourceID = _tokenContractAddressToResourceID[contractAddress];
bytes32 emptyBytes;
require(keccak256(abi.encodePacked((currentResourceID))) == keccak256(abi.encodePacked((emptyBytes))),
    "contract address already has corresponding resourceID");

A comparison of bytes32 to zero can be done directly (currentResourceID == bytes32(0)) without using hash functions.

Description

If there are not enough votes for a proposal to pass, that same proposal will be kept in the system forever and can, therefore, be voted on and executed later. The issue arises from the fact that it is impossible to cancel a proposal and be sure that it will never be executed, when you want to send a newer, updated one at a later point in time.

This issue applies to both EVM-compatible and Substrate-based chains.

Recommendation

A possible, and recommended, way to mitigate the issue is to introduce logic for proposals to have deadlines.