Fuel Labs Points Migration Function Security Review Report

The allowance increase is not necessary in this case because the old allowance will never be used by FuelERC20Gateway.

The stricter forceApprove() has fewer safety assumptions (no risk of leftover approvals usage) and uses less gas.

IERC20(token).safeIncreaseAllowance(
    fuelERC20Gateway,
    balanceAdjusted
);

Replace the safeIncreaseAllowance call with forceApprove.

The depositWithPermit function is designed to approve a spender by verifying a valid signature using the permit function. Most ERC20 tokens implement this functionality, allowing users to approve transfers without needing to send a separate transaction.

However, WETH (Wrapped Ether) does not implement the permit function. Despite this, when permit is called on the WETH token, the transaction executes without any errors because WETH's fallback function is triggered instead. This silent execution gives the appearance that the permit function succeeded, but in reality, the approval does not occur. As a result, the subsequent transfer in the depositWithPermit function will fail, causing the transaction to revert when the approval limit is insufficient.

function depositWithPermit(
    address token,
    uint240 amount,
    uint16 depositParam, // deprecated param
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
) external whenNotPaused {
    DepositRecord memory _tokenDeposit = deposits[_msgSender()][token];
 
    _tokenDeposit.balance += amount;
    _tokenDeposit.depositParam = depositParam;
    deposits[_msgSender()][token] = _tokenDeposit;
 
    // (hexens fuel4-2 fix: Permit can be ddos'ed)
    // Adding a try-catch clause allows to skip ddos transactions here
    // We are not interested in either catching the error or implementing
    // a success flow, we just continue and let safeTransferFrom revert
    try
        ERC20Permit(token).permit(
            _msgSender(),
            address(this),
            amount,
            deadline,
            v,
            r,
            s
        )
    {} catch {}
 
    IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);
    emit Deposit(_msgSender(), token, amount, depositParam);
}

Add a check in the depositWithPermit function to detect if the token being deposited is WETH. If WETH is detected, skip the permit function call entirely since it is not supported by WETH. Instead, require manual approval for WETH transfers.

The PreDeposits._migrate() will create an empty cross-chain transfer in case _adjustDepositDecimals returns zero for ETH or WETH, which is possible if the amount to send is less than 1e9.

Zero transfer in the case of other ERC20 tokens will result in a revert in FuelERC20GatewayV4, but FuelMessagePortal doesn't incorporate such checks.

uint256 balanceAdjusted = _adjustDepositDecimals(balance, token);

Add a check for balanceAdjusted > 0.

The decimals() function is not a part of the ERC-20 standard. It is just an optional extension - see here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Metadata.sol

Thus some ERC20 tokens may not support this interface, which potentially makes it unsafe to blindly cast all tokens to this interface.

Note that the PreDeposits.deposit() function will not revert for ERC20 tokens without the decimals() function, but when trying to migrate these tokens, it may revert.

function _adjustDepositDecimals(
    uint256 amount,
    address token
) internal returns (uint256) {
    uint256 tokenDecimals;
    if (token == address(0)) {
        tokenDecimals = 18;
    } else {
        tokenDecimals = IERC20Metadata(token).decimals();
    }

Consider handling the tokens without decimals() function.

In the PreDeposits contract the usage of msg.sender is inconsistent.

    IERC20(token).safeTransfer(msg.sender, dust);
} else {
    (bool sent, ) = address(msg.sender).call{ value: dust }(

Consider adjusting PreDeposits.sol by using _msgSender() instead of msg.sender for better consistency in the codebase.

When a user migrates their WETH from PreDeposits.sol, a Migration event may be emitted suggesting that ETH tokens address(0) were migrated, since token is set to address(0) in this case inside PreDeposits::_migrate() on line 163 in PreDeposits.sol.

if (token == address(weth)) {
    weth.withdraw(balance);
    token = address(0);
}
 
...
 
emit Migration(_msgSender(), token, balanceAdjusted);

Consider adjusting PreDeposits::_migrate() to emit the WETH token address when migrating WETH tokens.

PreDeposits::depositWithPermit() still sets the depositParam whereas PreDeposits::deposit() is setting depositParam to 0.

_tokenDeposit.depositParam = depositParam;

_tokenDeposit.depositParam = 0;

Inside PreDeposits::depositWithPermit() set depositParam to 0.

If a contract without receive() deposits WETH with some possible dust amount, it cannot use PreDeposits::migrate() since it will revert.

(bool sent, ) = address(msg.sender).call{ value: dust }(
    ""
);
if (!sent) {
    revert TransferReverted();
}

Consider mentioning the following for example in Fuel docs so that other developers are aware of this:

To avoid funds getting stuck, the contract which uses PreDeposits.sol with WETH should implement either:

1. receive function
2. a function to withdraw from PreDeposits
3. deposit without dust amount

In the PreDeposits contract, the receive() external payable {} function is used to allow the contract to accept ETH. However, if a user accidentally sends ETH to this contract, there is currently no mechanism to recover the funds, causing the ETH to become stuck in the contract.

receive() external payable {}

It is recommended to implement a recovery mechanism that allows for the retrieval of these funds.