fuellabs / fuel-bridge Goto Github PK

In the current implementation, even if there is a refund mechanism, loss of funds is possible by depositing to a fuel contract ID that does not exist in the Fuel blockchain. When a deposit transaction is called on the ERC20 Gateway, if there is an error on the fuelTokenId param, the message on the L2 will not ever be processed and thus a refund will not be triggered.

This can be alleviated by implementing a registry in the L1 contract. Upon deployment of the L2 counterpart, it is possible to send a message from L2 to L1, informing that the sender (the L2 contract) is able to process messages from the L1. Before accepting any deposit, the L1 contract can check if the destination fuelTokenId has previously reported its capabilities to process the deposits, which removes the possibility of this human error

Yesterday I made a Bridge transaction from my Fuel wallet to a a Sepolia Wallet I set up in Metamask.

Fuel wallet version: Version: 0.15.2

I clicked away from the bridge when it was in the state in screenshot above.

The transaction ID is: https://app.fuel.network/tx/0xcae664b7a7f8f4cd19ed3c4232b3a1d52fcdf798b36e99e9a29ff0f7235e7c0c/simple

I see that the transaction has taken place in my Fuel wallet:

from: fuel1dedeagaksypljjga2yy95dkxl2lk555dkujmu8qmrgerpxypvdvqxsl2ut (copied from fuel wallet)
to: 0x000000000000000000000000940aa7516018d954736c2ab1628337666f302fd8 (copied from fuel wallet)
and: 0x940AA7516018D954736C2ab1628337666F302fd8 (copied from Metamask wallet)

Only when I click to History do I see that I have to confirm the transaction.

When I click confirm Metamask pops up as if the Bridge is asking Metamask to sign the transaction.

but it cannot as there's not enough ETH in the wallet to do this.

but should this be happening at all? Surely the receiving wallet should not need to sign a transaction, or confirm an incoming transaction or have to pay a fee for an incoming transaction.

Is this a bug? Please let me know if I'm doing something wrong. Maybe the Bridge is opening Metamask when it should be opening the Fuel wallet extension. Or is this how it works with this kind of smart contract?

blocked by: FuelLabs/sway#4601, FuelLabs/fuels-ts#986

FuelLabs/sway-standards#21

Convert all bridging repos into a monorepo

Migrate the repo https://github.com/FuelLabs/fuel-merkle-sol to the fuel-bridge monorepo as this is an important package of the bridge infrastructure.

There is a low hanging fruit improvement for gas optimization: refactor the requirements to the if-revert-custom pattern.

https://soliditylang.org/blog/2021/04/21/custom-errors/

there is a problem when trying to withdraw from fuel portal bridge ,the first step which is submit to bridge keeps loading for hours and then nothing .

Description

Some parameters that are used in the merkle tree proofs are malleable for certain parts of the proof.
The serializeConsensusHeader function in the FuelERC20Gateway truncates the header.height of type uint64 to a uint32.

Figure 7.1: The serializeConsensusHeader function in fuel-v2-contracts/contracts/fuelchain/types/FuelBlockHeader.sol

function serializeConsensusHeader(FuelBlockHeader memory header) internal pure returns (bytes memory) {
    return
        abi.encodePacked(
            header.prevRoot,
            (uint32)(header.height),
            header.timestamp,
            computeApplicationHeaderHash(header)
        );
}

This means that the upper bits are malleable and changing them will result in the same hash. We were, however, not able to identify an attack vector.
In a similar manner, message.recipient is being truncated from bytes32 to address (20 bytes) in _executeMessage.

Figure 7.2: Snippet from the _executeMessage function in fuel-v2-contracts/contracts/fuelchain/FuelMessagePortal.sol

function _executeMessage(bytes32 messageId, Message calldata message) private nonReentrant {
    require(!_incomingMessageSuccessful[messageId], "Already relayed");

    //set message sender for receiving contract to reference
    _incomingMessageSender = message.sender;

    //relay message
    //solhint-disable-next-line avoid-low-level-calls
    bool success = SafeCall.call(
        address(uint160(uint256(message.recipient))),
        message.amount * (10 ** (ETH_DECIMALS - FUEL_BASE_ASSET_DECIMALS)),
        message.data
    );

In this case, changing the recipient bits would change the messageId required for the inclusion proof, meaning that this is not vulnerable in its current form.
We highlight these cases nonetheless, because changing code requirements and specifications can lead to mistakes.

Recommendations

Short term, do not truncate header.height and check that the upper bits for the message.recipient are zero.
Long term, aim to minimize any degrees of freedom that a user has on the system that could lead to a vulnerability.

Related partially to #50.

If the commitHeight % BLOCKS_PER_COMMIT_INTERVAL != 0, the blockHashAtCommit method returns an incorrect value.

I'm having trouble bridging from fuel --> sepolia.

Could it have to do with this....

https://support.metamask.io/hc/en-us/articles/360053147012-MetaMask-Legacy-Web3

This may not be implemented or stable in Sway at this moment but we should update for a performance boost.
Moreover, ref #10 in case that happens to be implemented.

Based of this suggestion: #40 (comment)

There is a limitation in the number of messages that can be sent in the tests. This might be a non-issue for now.

Coming from comment: #4 (comment)

bridgeERC20.ts is testing the deposit erc20 process, but when mounting the inputs to pay the transaction, it only considers Coin inputs.

• should add a test to validate if deposit erc20 token from CoinMessage works
  • bridge (deposit) ETH to a fuel account
  • then bridge(deposit) erc20 to the same fuel account

export function resourcesToInputs(resources: Array<Resource>) {
  const coinResources: Coin[] = resources.filter((r) =>
    isCoin(r)
  ) as unknown as Coin[];

  const coinInputs: Array<TransactionRequestInput> = coinResources.map(
    (r: Coin) => ({
      type: InputType.Coin,
      id: r.id,
      owner: r.owner.toB256(),
      amount: r.amount.toHex(),
      assetId: r.assetId,
      txPointer: ZeroBytes32,
      witnessIndex: 0,
    })
  );
  const messageCoinResources: MessageCoin[] = resources.filter((r) =>
    isMessage(r)
  ) as unknown as MessageCoin[];

  const messageCoinInputs: Array<TransactionRequestInput> =
    messageCoinResources.map((r: MessageCoin) => ({
      type: InputType.Message,
      assetId: r.assetId,
      sender: r.sender.toB256(),
      recipient: r.recipient.toB256(),
      nonce: r.nonce,
      amount: r.amount.toHex(),
      daHeight: r.daHeight.toHex(),
      witnessIndex: 0,
    }));
  // return coinInputs;
  return [...coinInputs, ...messageCoinInputs];
}

can you please add the testnet to orbiter finance so that we can use third party bridges also when the official bridge gas fee is high and it supports different l2s including starknet.

twitter: @Orbiter_Finance
link:- https://test.orbiter.finance

Currently some scripts are better suited to be run as hardhat tasks, with the main objective of leveraging the possibility of using cli args rather than having to configure inputs via env

Description

Missing validation in the blockhashAtCommit function results in an incorrect blockHash being returned for any commitHeight that is more than NUM_COMMIT_SLOTS behind the latest commit height. This might cause problems in frontend applications and other smart contract integrations that rely on the correct return value of this function.

Figure 1.1: The blockHashAtCommit function in fuel-v2-contracts/contracts/fuelchain/FuelChainState.sol:

116    function blockHashAtCommit(uint256 commitHeight) external view returns
(bytes32) {
117       Commit storage commitSlot = _commitSlots[commitHeight % NUM_COMMIT_SLOTS];
118       return commitSlot.blockHash;
119    }

The FuelChainState contract stores the latest NUM_COMMIT_SLOTS (figure 1.2) commits in an array (figure 1.3). Whenever a new commit is added a modulo operation is used to choose the correct index in this array (figure 1.4) at which to insert the new commit.

Figure 1.2: The NUM_COMMIT_SLOTS constant variable in fuel-v2-contracts/contracts/fuelchain/FuelChainState.sol

27    uint256 public constant NUM_COMMIT_SLOTS = 240; //30 days worth of commits

Figure 1.3: The _commitSlots array variable in fuel-v2-contracts/contracts/fuelchain/FuelChainState.sol

47    Commit[NUM_COMMIT_SLOTS] private _commitSlots;

Figure 1.4: The commit function in fuel-v2-contracts/contracts/fuelchain/FuelChainState.sol

88    function commit(bytes32 blockHash, uint256 commitHeight) external
whenNotPaused onlyRole(COMMITTER_ROLE) {
89       uint256 slot = commitHeight % NUM_COMMIT_SLOTS;
90       Commit storage commitSlot = _commitSlots[slot];
91       commitSlot.blockHash = blockHash;
92       commitSlot.timestamp = uint32(block.timestamp);
93
94       emit CommitSubmitted(commitHeight, blockHash);
95    }

If the blockHashAtCommit function is called with a commitHeight that is sufficiently far in the past, then the blockHash of another (a newer) commit will be returned. What instead should happen is that the function reverts if it cannot return the blockHash of the requested commit height.

Exploit Scenario

Alice, a developer of a frontend application that interacts with the Fuel smart contracts, develops a UI component to retrieve the block hash of at any given commit height. The UI component seems to work as it returns a block hash for every commit height that is requested. However, it shows an incorrect block hash for all sufficiently old commit heights.

Recommendations

Short term, update the blockHashAtCommit function to revert if it cannot return the blockHash of the requested commitHeight. For example, by adding a field height to the Commit struct and checking that commitHeight == commit.height.
Long term, take into consideration other applications that interact with the Fuel smart contracts and ensure that all view functions return correct results at all times.

• How the flow works?
• What happens if we commit a previous block that is already committed?

To improve the quality of Rust code we can add the following to CI

https://github.com/FuelLabs/sway-applications/blob/354bac23f11db6b2f89df53b46a8afe43c31911d/.github/workflows/ci.yml#L100-L103

Once we implemented #18 with a single contract for multiple tokens, we should create a deployment script for the fuel network side like what is inside the portal-contracts.

TOB Finding 3:

DescriptionWhen a refund is registered due to an error, any previous refund balance is overwrittenand lost.Refunds are issued when the bridged Ethereum asset is sent to an incompatible tokenaddress on the Fuel chain.

https://github.com/FuelLabs/fuel-bridge/blob/0861c0c57a3f3125f5ed28e16fdd883624343efd/packages/fungible-token/bridge-fungible-token/src/bridge_fungible_token.sw#L192C14-L192C14

TOB Finding 4:

DescriptionFunds in the FuelERC20Gateway can be drained due to returning an incorrect asset whenclaiming a refund.

https://github.com/FuelLabs/fuel-bridge/blob/0861c0c57a3f3125f5ed28e16fdd883624343efd/packages/fungible-token/bridge-fungible-token/src/bridge_fungible_token.sw#L132C18-L132C18

can you guys add owlto finance brdige support for the fuel network so that we guys can bridge eth to the fuel network using 3rd party bridge when there is high fee in official bridges.So that lot of web3 users can be attracted to the ecosysytem. And owlto finance does supports various L2's . please check it out once

twitter:-@Owlto_Finance
URL:- owlto.finance

I don't believe that you should be able to commit to the same height.

You commit once when you are ready. The challenge period is there. If it expires then you move on.
If someone invalidates then they should call some other function to clear the commit so that this function is usable again at that height.

Commentary is purely based on comment. I have not checked to see if this is already the case.

Originally posted by @Braqzen in #32 (comment)

Goal: We'd like to be able to deploy future testnet releases to mainnet, while doing everything we can to minimize any potential loss of funds for users due to upgrades, bugs, or other potential issues. This effort would be considered as adding "training wheels" to the bridge that could be removed once the network is stabilized and sufficiently tested.

High level solution:
Value cap the bridge such that users can only bring de minimis amounts of mainnet assets in the early stages. This will make it clear to users that the network is not stabilized yet, and in the case where funds cannot be easily recovered there would be no significant impacts. This could entail whitelisting which tokens are bridgeable, and placing a limit on each kind of asset based on an estimated fair market value. In addition to a global value restriction, there could also be account level restrictions to ensure each user is protected.

Description

Funds in the FuelERC20Gateway can be drained due to returning an incorrect asset when claiming a refund.
Refunds are registered when there is a mismatch between the L1 and L2 assets.

Refunds are accounted for in the register_refund function and stored in the storage variable refund_amounts.

Figure 4.2: The register_refund function in bridge-fungible-token/bridge-fungible-token/src/bridge_fungible_token.sw

// Storage-dependant private functions
#[storage(write)]
fn register_refund(from: b256, asset: b256, amount: b256) {
    storage.refund_amounts.get(from).insert(asset, amount);
    log(RefundRegisteredEvent {
        from,
        asset,
        amount,
    });
}

For a user to withdraw registered refunds the claim_refund function should be called. This function however incorrectly sets as the token to be withdrawn the BRIDGED_TOKEN instead of the asset variable.

Figure 4.3: The claim_refund function in bridge-fungible-token/bridge-fungible-token/src/bridge_fungible_token.sw

130    fn claim_refund(originator: b256, asset: b256) {
131       let stored_amount =
storage.refund_amounts.get(originator).get(asset).read();
132       require(stored_amount != ZERO_B256,
BridgeFungibleTokenError::NoRefundAvailable);
133
134       // reset the refund amount to 0
135       storage.refund_amounts.get(originator).insert(asset, ZERO_B256);
136
137       // send a message to unlock this amount on the base layer gateway contract
138       send_message(BRIDGED_TOKEN_GATEWAY, encode_data(originator, stored_amount,
BRIDGED_TOKEN), 0);
139    }

This allows an attacker to deposit any fake asset and to withdraw the true asset when claiming a refund.

Figure 4.4: The finalizeWithdrawal function in fuel-v2-contracts/contracts/messaging/gateway/FuelERC20Gateway.sol

    function finalizeWithdrawal(
        address to,
        address tokenId,
        uint256 amount
    ) external payable whenNotPaused onlyFromPortal {
        require(amount > 0, "Cannot withdraw zero");
        bytes32 fuelTokenId = messageSender();

        //reduce deposit balance and transfer tokens (math will underflow if amount is larger than allowed)
        _deposits[tokenId][fuelTokenId] = _deposits[tokenId][fuelTokenId] - amount;
        IERC20Upgradeable(tokenId).safeTransfer(to, amount);

        //emit event for successful token withdraw
        emit Withdrawal(bytes32(uint256(uint160(to))), tokenId, fuelTokenId, amount);
    }

This is possible because there is no deposit accounting per user in the FuelERC20Gateway which allows for withdrawals to different addresses other than the depositors.

Exploit Scenario

Alice creates a fake token and bridges 10K FAKE while specifying the Fuel ETH token id on the Fuel chain. Because the bridged asset is not appropriate, Alice is able to claim a refund. Due to the error, she is able to refund 10K ETH on Ethereum.

Recommendations

Short term, update the encoded data to include the deposited asset instead of the bridge asset. Alternatively, the gateway could be re-designed to not require specifying the Fuel token id on Ethereum which could eliminate the need for refunds.
Long term, pay extra attention to the intersection between blockchains. Make sure these are well-documented and tested, since they are typically harder to test and more prone to errors.

Original CI can be found here: https://github.com/FuelLabs/bridge-message-predicates/blob/master/.github/workflows/ci.yml
Crate: https://crates.io/crates/fuel-contract-message-predicate

Description

When a refund is registered due to an error, any previous refund balance is overwritten and lost.
Refunds are issued when the bridged Ethereum asset is sent to an incompatible token address on the Fuel chain.
Refunds are accounted for in the register_refund function and stored in the storage variable refund_amounts.

// Storage-dependant private functions
#[storage(write)]
fn register_refund(from: b256, asset: b256, amount: b256) {
    storage.refund_amounts.get(from).insert(asset, amount);
    log(RefundRegisteredEvent {
        from,
        asset,
        amount,
    });
}

Exploit Scenario

Alice bridges 1M USDC from Ethereum to the Fuel chain and includes a wrong recipient. Due to a mistake she sends the transaction twice. This results in her first deposit being lost.

Recommendations

Short term, increment the amounts when updating the refund storage values.
Long term, increase test coverage and make sure that special cases, such as triggering a refund case twice, are included.

Hey team, Pls I've used the fuel bridge multiple times before now and when I tried using it today, I didn't see my bridge history. I would like to to know if its cleared periodically or maybe its a bug

Description

Blocks that have not been committed yet could be seen as finalized.
If the Commit struct stored in _commitSlots is uninitialized, commitSlot.timestamp will default to zero and the boolean expression indicating whether a block is finalized given the commit’s timestamp will evaluate to true.

Figure 2.1: The finalized function in fuel-v2-contracts/contracts/fuelchain/FuelChainState.sol

101    /// @notice Checks if a given block is finalized
102    /// @param blockHash The hash of the block to check
103    /// @param blockHeight The height of the block to check
104    /// @return true if the block is finalized
105    function finalized(bytes32 blockHash, uint256 blockHeight) external view
whenNotPaused returns (bool) {
106       uint256 commitHeight = blockHeight / BLOCKS_PER_COMMIT_INTERVAL;
107       Commit storage commitSlot = _commitSlots[commitHeight % NUM_COMMIT_SLOTS];
108       require(commitSlot.blockHash == blockHash, "Unknown block");
109
110       return block.timestamp >= uint256(commitSlot.timestamp) +
111 }

However, in this case, the provided blockHash parameter must equal the default value of Commit.blockHash (bytes32(0)) for the function to pass the require-check on line 108.

Recommendations

Short term, include a check that returns false when commitSlot.timestamp equals zero.
Long term, carefully consider all edge cases, such as when checking for a finalized block when it has not been committed yet.

When building the bridge UI, we must have the contract types and message predicate bytecode. Both are currently not available on the npm package manager.

The packages should be released using what we already have on the structure.

• @fuel-bridge/message-predicate
• @fuel-bridge/portal-contracts

Coming from this comment:

#4 (comment)

Description

The Fuel ERC20 gateway contracts are able to receive Ether without the possibility of retrieving it.
The deposit, depositWithData and finalizeWithdrawal functions of the ERC20 gateway are all marked as payable, accepting Ether as payment.

Figure 6.1: The deposit function in fuel-v2-contracts/contracts/messaging/gateway/FuelERC20Gateway.sol

106    /// @dev Made payable to reduce gas costs
107    function deposit(bytes32 to, address tokenId, bytes32 fuelTokenId, uint256
amount) external payable whenNotPaused {
108       bytes memory messageData = abi.encodePacked(
109           fuelTokenId,
110           bytes32(uint256(uint160(tokenId))),
111           bytes32(uint256(uint160(msg.sender))), //from
112           to,
113           bytes32(amount)
114       );
115       _deposit(tokenId, fuelTokenId, amount, messageData);
116    }

Even though the contracts are not designed to handle Ether payments, the comment suggests that this has been done for gas optimization purposes. However, there exists no logic that would allow Ether that was accidentally sent to the contract to be retrieved.

Exploit Scenario

Alice calls the deposit function to deposit 1 WETH to the Fuel chain, but she accidentally also sends 1 Ether with the transaction. The transaction succeeds, and her WETH will be bridged. However, Alice is unable to retrieve the 1 Ether.

Recommendations

Short term, mark the functions as non-payable or include logic that would allow Ether to be retrieved.

Long term, carefully weigh the use of gas cost saving mechanisms against the footguns that such optimizations introduce. Protecting the user should be given priority over saving a small amount of gas.