users created message when withdrawing from L2-L1 is not possible to execute on L1 if the assetIssuerID got changed
Submitted on Sun Jul 14 2024 14:24:12 GMT-0400 (Atlantic Standard Time) by @zeroK for Attackathon | Fuel Network
Report ID: #33207
Report type: Smart Contract
Report severity: Insight
Impacts:
- Block stuffing
- Permanent freezing of funds
- Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
when users want to make withdraw call on L2(fuel) they should call the withdraw function which create a messageOut that correspond to the required data to call the relayMessage function on messagePortal, the sender of the messageOut should be set to the fuel bridge address so that the call to finalizedWithdraw which encoded as data can be passed, this is important step to bypass the message.sender == assetIssuerID check, however there is a function that updates the assetIssuerID to another 32 bytes address, this can cause lose of users funds because when users call withdraw function the sender of their message is equal to the bridge address which is the assetIssuerID and if the assetIssuer get Updated by the admin then old messages that set the bridge address as sender which is equal to the old assetIssuerID their message will never be able to executed as the message.sender == assetIssuerID will revert.
the reason we set this report to medium is that it depends on an action from the admin to update assetIssuerID innocently and cause user funds locked for ever.
this is how the withdraw message created:
#[payable]
#[storage(read, write)]
fn withdraw(to: b256) {
let amount: u64 = msg_amount();
require(amount != 0, BridgeFungibleTokenError::NoCoinsSent);
let asset_id = msg_asset_id();
let sub_id = _asset_to_sub_id(asset_id);
let token_id = _asset_to_token_id(asset_id);
let l1_address = _asset_to_l1_address(asset_id);
// Hexens Fuel1-4: Might benefit from a custom error message
storage
.tokens_minted
.insert(
asset_id,
storage
.tokens_minted
.get(asset_id)
.read() - amount,
);
burn(sub_id, amount);
// send a message to unlock this amount on the base layer gateway contract
let sender = msg_sender().unwrap();
send_message(
BRIDGED_TOKEN_GATEWAY,
encode_data(to, amount.as_u256().as_b256(), l1_address, token_id), // this will be the finalizedWithdraw 4 bytes + inputs
0,
);
log(WithdrawalEvent {
to: to,
from: sender,
amount: amount,
});
}
this call will set the message.sender to the l2 address that mint token or the assetIssuerID and when user try to execute the call to relayMessage on the message portal contract:
function relayMessage(
Message calldata message,
FuelBlockHeaderLite calldata rootBlockHeader,
FuelBlockHeader calldata blockHeader,
MerkleProof calldata blockInHistoryProof,
MerkleProof calldata messageInBlockProof
) external payable virtual override whenNotPaused {
if (withdrawalsPaused) {
revert WithdrawalsPaused();
}
//verify root block header
if (!_fuelChainState.finalized(rootBlockHeader.computeConsensusHeaderHash(), rootBlockHeader.height)) {
revert UnfinalizedBlock();
} // if not finalized then revert
//verify block in history
if (
!verifyBinaryTree(
rootBlockHeader.prevRoot,
abi.encodePacked(blockHeader.computeConsensusHeaderHash()),
blockInHistoryProof.proof,
blockInHistoryProof.key,
rootBlockHeader.height
)
) revert InvalidBlockInHistoryProof();
//verify message in block
bytes32 messageId = CryptographyLib.hash(
abi.encodePacked(message.sender, message.recipient, message.nonce, message.amount, message.data)
);
if (messageIsBlacklisted[messageId]) {
revert MessageBlacklisted();
}
if (
!verifyBinaryTree(
blockHeader.outputMessagesRoot,
abi.encodePacked(messageId),
messageInBlockProof.proof,
messageInBlockProof.key,
blockHeader.outputMessagesCount
)
) revert InvalidMessageInBlockProof();
//execute message
_executeMessage(messageId, message);
}
/// @notice Executes a message in the given header
/// @param messageId The id of message to execute
/// @param message The message to execute
function _executeMessage(bytes32 messageId, Message calldata message) internal virtual override nonReentrant {
if (_incomingMessageSuccessful[messageId]) revert AlreadyRelayed();
//set message sender for receiving contract to reference
_incomingMessageSender = message.sender;
// v2: update accounting if the message carries an amount
bool success;
bytes memory result;
if (message.amount > 0) {
uint256 withdrawnAmount = message.amount * PRECISION;
// Underflow check enabled since the amount is coded in `message`
totalDeposited -= withdrawnAmount;
(success, result) = address(uint160(uint256(message.recipient))).call{value: withdrawnAmount}(message.data);
} else {
(success, result) = address(uint160(uint256(message.recipient))).call(message.data); // call finalizedWithdraw
}
if (!success) {
// Look for revert reason and bubble it up if present
if (result.length > 0) {
// The easiest way to bubble the revert reason is using memory via assembly
/// @solidity memory-safe-assembly
assembly {
let returndata_size := mload(result)
revert(add(32, result), returndata_size)
}
}
revert MessageRelayFailed();
}
//unset message sender reference
_incomingMessageSender = NULL_MESSAGE_SENDER;
//keep track of successfully relayed messages
_incomingMessageSuccessful[messageId] = true;
//emit event for successful message relay
emit MessageRelayed(messageId, message.sender, message.recipient, message.amount);
}
we can see the _incomingMessageSender is set to message.sender which is same as assetIusserID, however if the admin make call to the function then any message created on L2 and ready to execute on L1 by calling relayMessage will revert because the assetIssuerID is no longer equal to the old one or message.sender to messages that created before updating the assetIssuerID.
/// @notice sets the entity on L2 that will mint the tokens
function setAssetIssuerId(bytes32 id) external payable virtual onlyRole(DEFAULT_ADMIN_ROLE) {
assetIssuerId = id;
}
// the call to finalizeWithdraw will revert because of the update assetIssuerID after creating message according to the old one
/// @notice Finalizes the withdrawal process from the Fuel side gateway contract
/// @param to Account to send withdrawn tokens to
/// @param tokenAddress Address of the token being withdrawn from Fuel
/// @param l2BurntAmount Amount of tokens to withdraw
/// @dev Made payable to reduce gas costs
function finalizeWithdrawal(
address to,
address tokenAddress,
uint256 l2BurntAmount,
uint256 /*tokenId*/
) external payable virtual override whenNotPaused onlyFromPortal {
if (l2BurntAmount == 0) {
revert CannotWithdrawZero();
}
if (messageSender() != assetIssuerId) {
revert InvalidSender();
}
uint8 decimals = _getTokenDecimals(tokenAddress);
uint256 amount = _adjustWithdrawalDecimals(decimals, l2BurntAmount);
//reduce deposit balance and transfer tokens (math will underflow if amount is larger than allowed)
_deposits[tokenAddress] = _deposits[tokenAddress] - l2BurntAmount;
IERC20MetadataUpgradeable(tokenAddress).safeTransfer(to, amount);
//emit event for successful token withdraw
emit Withdrawal(bytes32(uint256(uint160(to))), tokenAddress, amount);
}
while the function exist its mean there is a point when the admin want to change the assetIssuerID and if this happen while the message that created depends on the old assetIssuerID will never be executed and users on L2 burn their tokens for nothing
loss/stuck of users funds when the assetIssuerID get updated.
its recommended to add functionality to the bridges contract to allow updating the messages that executed depends on previous L2 minter address/assetIssuerID when the assetIssuerID get updated when required, the functionality should have sanity checks that only allow updating message data and re execute it when :
-
the assetIssuerID updated.
-
the message not executed yet.
on L2 POC:
contract;
use standards::src10::SRC10;
abi MyContract {
fn call_withdraw();
}
impl MyContract for contract {
fn foo(to_address: b256, bridge: ContractId, bridged_asset: AssetId) {
let bridge_abi = abi(SRC10, bridge.value);
bridge_abi {
gas: 10000,
coins: 100,
asset_id: bridged_asset,
}.withdraw(to_address);
return true;
}
}
#[test]
fn test_contract() {
let caller = abi(MyContract, CONTRACT_ID);
let to = 0xf8f8b6283d7fa5b672b530cbb84fcccb4ff8dc40f8176ef4544ddb1f1952ad07
let contract_ids = ContractID::this();
let asset_id = b256::zero();
let result = caller.withdraw(to, contract_ids, asset_id);
assert(result == true)
}
// this will show killed because of an issue in forc the call to update the assetIssuerID on L1 POC:
// SPDX-License-Identifier: BSD 3-Clause License
pragma solidity ^0.8.15;
import "forge-std/Test.sol";
import "./interfaces/IPortalMessage.sol";
import "./interfaces/IERC20.sol";
// run this test in this repo : https://github.com/SunWeb3Sec/defilabs run forge test --contracts src/test/test.sol -vvvv
contract ContractTest is Test {
event LogBytes(bytes32 data);
address public alice;
address public bob;
bytes32 public assetIssuerID;
FuelMessagePortalV3 fuelPortal =
FuelMessagePortalV3(0x01855B78C1f8868DE70e84507ec735983bf262dA);
address usdt = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
enum MessageType {
DEPOSIT_TO_ADDRESS,
DEPOSIT_TO_CONTRACT,
DEPOSIT_WITH_DATA,
METADATA
}
function setUp() public {
vm.createSelectFork("sepolia");
alice = vm.addr(1);
bob = vm.addr(2);
deal(address(alice), 1 ether);
}
function testExploitHere() public {
//STEP 1 only to show the id before and after
bytes32 id1 = bytes32(uint256(uint160(bob)));
setAssetIssuerID(id1);
uint256 l2MintedAmount = 100 * 10 ** 9;
bytes memory depositMessage1 = abi.encodePacked(
assetIssuerID, // this will be 0000
uint256(MessageType.DEPOSIT_TO_ADDRESS),
bytes32(uint256(uint160(usdt))),
uint256(0), // token_id = 0 for all erc20 deposits
bytes32(uint256(uint160(msg.sender))),
alice,
l2MintedAmount,
uint256(18)
); // not important for this POC
emit LogBytes(id1); // log 32 bytes
//STEP 2: after user initialized transaction to withdraw on L2 with setting the the old L2 bridge address(assetIssuerID) the admin updated the assetIssuer which block user to withdraw their tokesn in L1
bytes32 id2 = bytes32(uint256(uint160(alice)));
setAssetIssuerID(id2);
emit LogBytes(id2);
}
function setAssetIssuerID(bytes32 id) public {
assetIssuerID = id;
}
}