Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TOB-FUEL-7: Merkle tree proof parameters could be malleable #56

Closed
xgreenx opened this issue Aug 26, 2023 · 0 comments · Fixed by #62
Closed

TOB-FUEL-7: Merkle tree proof parameters could be malleable #56

xgreenx opened this issue Aug 26, 2023 · 0 comments · Fixed by #62
Labels
audit-report Related to the audit report

Comments

@xgreenx
Copy link
Contributor

xgreenx commented Aug 26, 2023

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.

@xgreenx xgreenx added the audit-report Related to the audit report label Aug 26, 2023
@DefiCake DefiCake mentioned this issue Sep 13, 2023
DefiCake added a commit that referenced this issue Sep 14, 2023
Closes #56 

After checking possible side effects, I could only come up with the fact
that `relayMessage` - the function used by users to withdraw from L2 to
L1 - will change its function signature as its input type will be
changed. This will warrant the change in whatever frontend that might be
using this function.

This change also brings the solidity typings closer to the [fuel-core
typings](https://github.com/FuelLabs/fuel-core/blob/7934cbd05900169a7ed24f78e09fdab04718ec74/crates/types/src/blockchain/header.rs#L91C23-L91C28),
as `height` is an `uint32` type in the consensus. I am unsure about the
reasoning behind it being `uint64` in solidity, just to truncate it
later to `uint32`. Was it originally `u64` in the consensus spec? Maybe
@Voxelot can bring some light into it.

EDIT. [So it seems that this is a remnant from a change of
spec](FuelLabs/fuel-specs@667429c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Related to the audit report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant