OFT

In the FunOAppComposer contract lzCompose function there are several cases where the to address is allowed to be any arbitrary contract, along with any arbitrary calldata:

(bool success, bytes memory data) = address(_to).call(_callData);

The composer has a case to handle ERC20 OFTs which means it will have a nonzero balance for these ERC20 OFTs when they have been delivered but not yet had their composed message executed.

The arbitrary call to an arbitrary address however allows all ERC20 tokens sitting in the Composer to be stolen. This occurs when a malicious actor provides the token address as the to and provides the calldata of approve(...) to approve themselves to spend the entire composer contract's balance.

Consider removing the support of ERC20 OFTs and also removing all calls where the to address is not explicitly the composerConfig.VaultWrapper.

As mentioned in L-04, there is a griefing vector where the composer is trusting that other delegate depositors do not instantiate alternate subaccounts for the user as this would DoS the composed message for eternity, trapping user funds.

However the FunOAppComposer contract could itself be used to grief accounts in this way. This is because the composed message is what provides the arbitrary calldata which will hold the details of which subaccount and account to deposit into. Any malicious actor could create a composed message that routes through the FunOappComposer system and depositOnBehalf’s a dust amount of USDe for an unsuspecting account who also has an in-flight compose message.

Execution of compose messages is permissionless, so it is easily possible that a malicious actor could execute theirs through the LZ EndpointV2 contract before the user’s honest depositOnBehalf action is executed, thus trapping their cross-chain deposit amounts forever.

Consider requiring that the compose message also includes a signature, which validates that the compose message provided is indeed attested for by the account that is being deposited on behalf of. This signature will include attestation for the subaccount, deposit amount, and deposit token so there is no way to DoS the user’s lzCompose by making the same deposit.

Ethereal requires that only whitelisted delegate depositors can invoke the depositOnBehalf function because there is a risky griefing vector associated with it, whereby a user can DoS the depositOnBehalf flow for an account by registering an unexpected subaccount before the user's cross-chain depositOnBehalf is completed.

The VaultDepositooor contract will be whitelisted for the delegate depositor permission on Ethereal, and also contains a permissionless deposit function which opens this access up to anyone, with arbitrary calldata going to the depositOnBehalf function.

Make the VaultDepositooor deposit function whitelisted so that only the composer contract can invoke it.

Now included in the composeMsg is a nonce value, however this nonce value and the associated check are not necessary as they don't prevent any kind of replay attack as there is no longer any signature of the composeMsg.

This nonce check in fact creates opportunities for user's to have their funds trapped by accidentally using the same nonce in the composeMsg.

Remove all associated nonce logic.

The depositOnBehalf function is not allowed when the account has not registered the corresponding subaccount and the account itself has already been initialized.

In other words, if the account already has a separate subaccount that is active, the deposit is not allowed.

This can cause the lzCompose action to be DoS’d in the case where the user creates a deposit for a different subaccount themselves, or another delegateDepositor creates a separate subaccount for the user before the lzCompose action is executed.

In this scenario, the user’s funds in the cross-chain bridge would be permanently trapped.

Understand that you are trusting the other whitelisted delegate depositors to not DoS the lzCompsoe actions this way, and warn users that they should never make a direct deposit to Ethereal before their bridge and lzCompose action is completed.

In the lzCompose function the application requires that the provided _amount inside the user specified _message composed message is exactly equal to the OFT amount that was sent and is recorded as a part of the base composeMsg in OFTComposeMsgCodec.amountLD(_message).

However if an amount that is different than the exact transferred amount is errantly supplied, this will cause the compose execution to fail and the users funds to then be trapped in the composer.

Consider removing the ability to even specify the redundant _amount value in the composed message contents and simply rely on the OFTComposeMsgCodec.amountLD(_message) as the single source of truth.

The Ethereal depositOnBehalf function accepts a list of DepositOnBehalfRequest objects to initiate multiple depositOnBehalf actions.

However multiple request objects is incompatible with the VaultDepositooor system as it only replaces the first instance of the AMOUNT_PLACEHOLDER in the provided call data.

Ensure that in the frontend and all flows routing through the FunOappComposer that only one such DepositOnBehalfRequest entry is used.

The Ethereal system has a cap for deposits of any particular asset, for which only USDe is enabled at the moment.

This means that a deposit through the Fun system that would put the Ethereal system right at the cap, or at least near it, could be DoS’d by sending a small amount of native tokens directly to the Depositooor contract right before the lzCompose action is called.

The lzCompose message would then be stuck until that additional balance in the Depositooor contract is cleared.

Simply be aware of this issue, it is unlikely that such a cap will be reached with a Fun deposit and that a user would grief the system like this.

In any lzComposer it is adamant that the call data provided is correctly formatted and contains accurate data. If anything causes a revert in the decoding, or application of the call data — whether that be in the composer contract or the downstream Ethereal contracts, this will permanently trap user funds in the composer contract.

Furthermore, no rescue functions should be added, since for some errors they may only be temporary, and if you were to rescue funds it would then allow users to steal assets from other bridges once their lzCompose message does become executable.

Be extremely cautious about the call data that is provided to the system, it should not trigger any of the revert cases that Ethereal holds in it’s depositOnBehalf flow such as the following amongst several others:

uint256 depositsLength = deposits.length;
if (depositsLength == 0) {
    revert ZeroLength();
}

uint16 maxBatchSize = Exchange.load().delegateDepositMaxBatchSize;
if (depositsLength > maxBatchSize) {
    revert DepositOnBehalfBatchSizeExceeded(maxBatchSize);
}
ICollateralManager.DepositOnBehalfRequest calldata d = deposits[i];
if (d.tokenName != USD_TOKEN_NAME) {
    revert InvalidDepositOnBehalf(msg.sender, d.account, d.subaccount, "INVALID_TOKEN");
}
if (d.account == address(0)) {
    revert ZeroAddress();
}

if (amount == 0) {
    revert ZeroAmount();
}
if (subaccount == bytes32(0)) {
    revert ZeroLength();
}
if (depositToken == address(0)) {
    revert InvalidDepositToken(depositToken);
}

The depositOnBehalf function may refund a dust amount back to the FunOappComposer but no logic is implemented to sweep it. These funds are therefore locked in the contract and accumulate over time.

Be aware about this behavior and consider to document it.

The constructor of the FunOappComposer contract saves the given composerConfig without performing zero address checks.

Consider to perform zero address checks to follow best practices.

The NatSpec for the constructor still references the VaultWrapper contract, however this is no longer used in the contract flow.

Update the NatSpec throughout the composer contract.