Fuel Labs Bridge Contracts Update Security Review Report

The introduced rate limits in FuelERC20GatewayV4 and FuelMessagePortalV3 can get a withdrawal stuck if the amount is larger than the rate limit allowance per period.

E.g., if the rate limit is 100^18 per 1 hour, and there is a withdrawal of 100^18 + 1 tokens, the call to finalizeWithdrawal() will always fail. The withdrawal will require manual adjustment of the rate limit for each individual case, which can lead to other participants using more than the desired limit and also operation losses for the victims because of any execution delays and the team because of the gas fees.

    function _addWithdrawnAmount(address _token, uint256 _withdrawnAmount) internal {
        uint256 currentPeriodAmountTemp;
 
        if (currentPeriodEnd[_token] < block.timestamp) {
            unchecked {
                currentPeriodEnd[_token] = block.timestamp + rateLimitDuration[_token];
            }
            currentPeriodAmountTemp = _withdrawnAmount;
        } else {
            unchecked {
                currentPeriodAmountTemp = currentPeriodAmount[_token] + _withdrawnAmount;
            }
        }
 
        if (currentPeriodAmountTemp > limitAmount[_token]) {
            revert RateLimitExceeded();
        }
 
        currentPeriodAmount[_token] = currentPeriodAmountTemp;
    }

    function _addWithdrawnAmount(uint256 _withdrawnAmount) internal {
        uint256 currentPeriodAmountTemp;
 
        if (currentPeriodEnd < block.timestamp) {
            unchecked {
                currentPeriodEnd = block.timestamp + rateLimitDuration;
            }
            currentPeriodAmountTemp = _withdrawnAmount;
        } else {
            unchecked {
                currentPeriodAmountTemp = currentPeriodAmount + _withdrawnAmount;
            }
        }
 
        if (currentPeriodAmountTemp > limitAmount) {
            revert RateLimitExceeded();
        }
 
        currentPeriodAmount = currentPeriodAmountTemp;
    }

We recommend:

• addition of the rate limits logic on the Fuel side as well
• or addition of an automated refund system in case the withdrawal is too large
• or addition of a queue system and partial withdrawals (this will be complicated in the case of ETH because of the callback)

Commentary from the client:

We are considering a fix but we have not yet clear path way on implementing them, our bandwidth is limited with the mainnet launch. But we will definitely consider a fix long term.

We probably are going to put a withdraw limiter on the L2 contracts.

The rate limits introduced in FuelERC20GatewayV4 and FuelMessagePortalV3 can be used by malicious actors to DoS the ETH and ERC20 (if enabled) withdrawals.

The attackers simply need to withdraw the amount equal to a limit and deposit it back at the start of every rate limit period.

The attack is relatively cheap to perform, as the costs would only be the bridging fees to and from Fuel (a couple of dollars per period).

    function _addWithdrawnAmount(address _token, uint256 _withdrawnAmount) internal {
        uint256 currentPeriodAmountTemp;
 
        if (currentPeriodEnd[_token] < block.timestamp) {
            unchecked {
                currentPeriodEnd[_token] = block.timestamp + rateLimitDuration[_token];
            }
            currentPeriodAmountTemp = _withdrawnAmount;
        } else {
            unchecked {
                currentPeriodAmountTemp = currentPeriodAmount[_token] + _withdrawnAmount;
            }
        }
 
        if (currentPeriodAmountTemp > limitAmount[_token]) {
            revert RateLimitExceeded();
        }
 
        currentPeriodAmount[_token] = currentPeriodAmountTemp;
    }

    function _addWithdrawnAmount(uint256 _withdrawnAmount) internal {
        uint256 currentPeriodAmountTemp;
 
        if (currentPeriodEnd < block.timestamp) {
            unchecked {
                currentPeriodEnd = block.timestamp + rateLimitDuration;
            }
            currentPeriodAmountTemp = _withdrawnAmount;
        } else {
            unchecked {
                currentPeriodAmountTemp = currentPeriodAmount + _withdrawnAmount;
            }
        }
 
        if (currentPeriodAmountTemp > limitAmount) {
            revert RateLimitExceeded();
        }
 
        currentPeriodAmount = currentPeriodAmountTemp;
    }

Commentary from the client:

We are considering a fix but we have not yet clear path way on implementing them, our bandwidth is limited with the mainnet launch. But we will definitely consider a fix long term.

Is really a trade-off, we cannot deal with it without removing the feature completely. We prefer to deal with DoS issues.

Status: Fixed

Inside the function FuelMessagePortalV3::resetRateLimitAmount(), when the period has not expired and the new limit amount is less than the withdrawn amount in this period, the state var currentPeriodAmount is assigned to the new limitAmount (line 104 FuelMessagePortalV3.sol).

Normally this won't cause any problem, but in the following scenario, it may behave unexpectedly: the SET_RATE_LIMITER_ROLE sets the limit amount to a lower value and then they set it to a higher value.

For example, let's say the initial limit amount was 10e18 and in this period the amount withdrawn currentPeriodAmount is also 10e18.

Then the SET_RATE_LIMITER_ROLE sets a new limit amount to 3e18. The protocol will adjust the currentPeriodAmount to 3e18.

However the SET_RATE_LIMITER_ROLE realizes that their intention was actually setting it to 9e18 and they call resetRateLimitAmount() (within the same period) again with 9e18.

This will be a problem, since in this period additional 6e18 can be withdrawn, because currentPeriodAmount is not updated when the limitAmount is then set to 9e18.

Additionally, the SET_RATE_LIMITER_ROLE might not expect that setting the limitAmount from 10e18 to 3e18 and then to 9e18 will behave differently from setting the limitAmount directly to 9e18.

FuelERC20GatewayV4::resetRateLimitAmount() (FuelERC20GatewayV4.sol#L156-L188) has almost identical code, so the same issue also exists there.

    /**
     * @notice Resets the rate limit amount.
     * @param _amount The amount to reset the limit to.
     */
    function resetRateLimitAmount(uint256 _amount) external onlyRole(SET_RATE_LIMITER_ROLE) {
        uint256 withdrawalLimitAmountToSet;
        bool amountWithdrawnLoweredToLimit;
        bool withdrawalAmountResetToZero;
 
        // if period has expired then currentPeriodAmount is zero
        if (currentPeriodEnd < block.timestamp) {
            unchecked {
                currentPeriodEnd = block.timestamp + rateLimitDuration;
            }
            withdrawalAmountResetToZero = true;
        } else {
            // If the withdrawn amount is higher, it is set to the new limit amount
            if (_amount < currentPeriodAmount) {
                withdrawalLimitAmountToSet = _amount;
                amountWithdrawnLoweredToLimit = true;
            }
        }
 
        limitAmount = _amount;
 
        if (withdrawalAmountResetToZero || amountWithdrawnLoweredToLimit) {
            currentPeriodAmount = withdrawalLimitAmountToSet;
        }
 
        emit ResetRateLimit(_amount);
    }

Do not adjust currentPeriodAmount inside FuelMessagePortalV3::resetRateLimitAmount() when the period has not expired and the new limit amount is less than the withdrawn amount in this period.

Status: Fixed

In FuelChainState.sol, the constructor does not validate that timeToFinalize, blocksPerCommitInterval, and commitCooldown are non-zero. This could result in misconfigurations or unintended behavior if any of these values are set to 0.

    constructor(uint256 timeToFinalize, uint256 blocksPerCommitInterval, uint32 commitCooldown) {
        if (timeToFinalize > commitCooldown) {
            revert FinalizationIsGtCooldown();
        }
        uint256 circularBufferSizeInSeconds = NUM_COMMIT_SLOTS * blocksPerCommitInterval;
 
        if (timeToFinalize > circularBufferSizeInSeconds) {
            revert TimeToFinalizeTooLarge();
        }
 
        if (commitCooldown > circularBufferSizeInSeconds) {
            revert CommitCooldownTooLarge();
        }
 
        TIME_TO_FINALIZE = timeToFinalize;
        COMMIT_COOLDOWN = commitCooldown;
        BLOCKS_PER_COMMIT_INTERVAL = blocksPerCommitInterval;
 
        _disableInitializers();
    }

Add a check specifically for timeToFinalize, as ensuring it is non-zero will indirectly handle issues with other values like blocksPerCommitInterval and commitCooldown, as they will naturally revert through the existing logic if they are zero:

require(timeToFinalize > 0, "timeToFinalize cannot be zero");

Status: Fixed, commits: 1, 2

The function responsible for the rate limit updates in FuelMessagePortalV3 and FuelERC20GatewayV4, _addWithdrawnAmount(), will always be called once activated on every withdrawal. Even if the limit is raised to uint256.max, users would still pay about 3000 gas for each transaction.

        _addWithdrawnAmount(withdrawnAmount);

        if (currentPeriodEnd[tokenAddress] != 0) _addWithdrawnAmount(tokenAddress, amount);

We recommend adding a function that will disable the rate limit or adding a special case when it'll be skipped.

Status: Fixed, commits: 1, 2, 3

The resetRateLimitAmount() function in both FuelMessagePortalV3 and FuelERC20GatewayV4 will set the new limit immediately to the current period if it hasn't yet ended.

The function in FuelERC20GatewayV4 can also update the limit duration. A new higher limit amount and duration (the limit is updated less frequently but with larger amounts) will lead to the possibility of withdrawing a large amount of ERC20 tokens in a short period of time.

For example, suppose the old period is 1 token per 120 seconds, and the new one is 1000 tokens per day. At the moment of change, users can withdraw 1000 tokens in the remaining 120-second period as if it was updated retroactively and lasted a day.

In FuelMessagePortalV3, this can also lead to a momentary increase in withdrawals, but it does not pose an issue as the time periods will stay the same.

    function resetRateLimitAmount(address _token, uint256 _amount, uint256 _rateLimitDuration) external onlyRole(SET_RATE_LIMITER_ROLE) {
        uint256 withdrawalLimitAmountToSet;
        bool amountWithdrawnLoweredToLimit;
        bool withdrawalAmountResetToZero;
 
        // avoid multiple SLOADS
        uint256 rateLimitDurationEndTimestamp = currentPeriodEnd[_token];
 
        // set new rate limit duration
        rateLimitDuration[_token] = _rateLimitDuration;
 
        // if period has expired then currentPeriodAmount is zero
        if (rateLimitDurationEndTimestamp < block.timestamp) {
            unchecked {
                currentPeriodEnd[_token] = block.timestamp + _rateLimitDuration;
            }
            withdrawalAmountResetToZero = true;
        } else {
            // If the withdrawn amount is higher, it is set to the new limit amount
            if (_amount < currentPeriodAmount[_token]) {
                withdrawalLimitAmountToSet = _amount;
                amountWithdrawnLoweredToLimit = true;
            }
        }
 
        limitAmount[_token] = _amount;
 
        if (withdrawalAmountResetToZero || amountWithdrawnLoweredToLimit) {
            currentPeriodAmount[_token] = withdrawalLimitAmountToSet;
        }
 
        emit RateLimitUpdated(_token, _amount);
    }

We recommend the addition of a temporary variable to store the new limit until the end of the current period or start the new period immediately during the call to the reset function.

Status: Fixed

The comment in FuelERC20GatewayV4.sol on line 80 for the rateLimitDuration is incorrect.

    /// @notice Amounts already withdrawn this period for each token.
    mapping(address => uint256) public rateLimitDuration;

Consider fixing the comment for the rateLimitDuration.

The scripts of the previous testnets include a call to pause withdrawals right after the upgrade to FuelMessagePortal.

Given that the upgrade and the pausing will be performed as separate transactions, it will still be possible to withdraw in between.

const portal = FuelMessagePortalV3.connect(address, deployer);
 
await portal
  .pauseWithdrawals()
  .then((tx) => {
    console.log('Pausing withdrawals...', tx.hash);
    return tx.wait();
  })
  .then(() => console.log('Withdrawals paused'));
 
return true;

The SRC14 docs (https://docs.fuel.network/docs/sway-standards/src-14-simple-upgradeable-proxies/#optional-public-functions) are recommending:

The following functions are RECOMMENDED to be implemented by a proxy contract to follow the SRC-14 standard:

fn proxy_owner() -> State;

However, packages/fungible-token/bridge-fungible-token/proxy/src/proxy.sw implements the function with a leading underscore as fn _proxy_owner().

impl Proxy for Contract {
    #[storage(read)]
    fn _proxy_owner() -> State {
        let owner = storage::SRC14.owner.read();
 
        match owner {
            State::Uninitialized => INITIAL_OWNER,
            _ => owner,
        }
    }

Consider following the docs by updating the function name _proxy_owner() to proxy_owner().