WormholeDepositProcessorL1 never sends cross-chain messages and bridges tokens only to EVM chains

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/staking/WormholeDepositProcessorL1.sol#L96-L97

Vulnerability details

Relevant code: WormholeDepositProcessorL1::_sendMessage

Description

The deposit processors used for bridging on L1 work in 2 possible ways:

• They can send tokens
• Or send a message to L2 (in case of enough withheld tokens)

Some examples are GnosisDepositProcessorL1::_sendMessage:

if (transferAmount > 0) {
    ...
    bytes memory data = abi.encode(targets, stakingIncentives);
    IBridge(l1TokenRelayer).relayTokensAndCall(olas, l2TargetDispenser, transferAmount, data); 
    ...
} else {
    ...
    bytes32 iMsg = IBridge(l1MessageRelayer).requireToPassMessage(l2TargetDispenser, data, gasLimitMessage);
    ...
}

ArbitrumDepositProcessorL1::_sendMessage:

if (transferAmount > 0) {
    ...
    IBridge(l1TokenRelayer).outboundTransferCustomRefund{value: cost[0]}(olas, refundAccount,
        l2TargetDispenser, transferAmount, TOKEN_GAS_LIMIT, gasPriceBid, submissionCostData);
}
...
sequence = IBridge(l1MessageRelayer).createRetryableTicket{value: cost[1]}(l2TargetDispenser, 0,
    maxSubmissionCostMessage, refundAccount, refundAccount, gasLimitMessage, gasPriceBid, data);

The same is true for Optimism and Polygon processors as well.

The only deposit processor that is not implemented like this is WormholeDepositProcessorL1. This means that every time we might want to only send a message, the token bridging flow will be triggered, which wastes additional resources on every call.

The other issue is that according to the Readme, one of the chains Olas is going to be deployed to is Solana. WormholeDepositProcessorL1 is the only way to send messages/tokens to the Solana chain. Thus, it should use the corresponding transferTokens function when wormholeTargetChainId points to a non-evm chain.

Root Cause

1. Using token bridging when a simple cross-chain message is enough. The following function is always called:

   sequence = sendTokenWithPayloadToEvm(uint16(wormholeTargetChainId), l2TargetDispenser, data, 0, gasLimitMessage, olas, transferAmount, uint16(l2TargetChainId), refundAccount);

2. Always making function call for EVM-based chain, even though its possible that wormholeTargetChainId == 1 (Solana)

   Impact

   Users claiming fees for Staking contracts are forced to always use the token bridging flow and pay higher transaction fees than necessary. Over time overpaying in fees will sum up to a sizeable amount, especially considering the high dependency on cross-chain transactions.

WormholeDepositProcessorL1 does not work with non-EVM functions, even though it is planned to do so. This can cause loss of funds when distributing token incentives.

Suggested Mitigation

The l1MessageRelayer is able to send messages to EVM and non-EVM chains via sendPayloadToEvm and send respectively when transferAmount == 0.

Use transferTokens function call when bridging tokens to non-EVM chains.

Assessed type

Other

[kupermind]

The designed set of Wormhole bridging contracts is developed for EVM chains only. Those would be totally different for Solana or other non-EVM chains. The Solana and other non-EVM chains only have an interface in the Dispenser for future, however as one could observe there is nothing yet implemented for those chains, same goes for cross-chain bridging interactions.

[c4-sponsor]

kupermind (sponsor) disputed

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Overinflated severity

[0xA5DF]

As sponsor said, the code is clearly not intended for Solana The suggestion for a more efficient sending, if true, might be a QA at best

[georgiIvanov]

Hello. As it is stated in the contest readme scoping section, the project will be deployed to Solana:

Furthermore, nowhere in the codebase or Publicly Known Issues section was it documented that bridging to Solana is not supported.

This puts non-evm bridging via wormhole in scope because it was allowed in readme and the comment from the sponsor above wasn't specified anywhere - code or repo docs.

At the same time, code like this exists in Dispenser:

else {
    // Send to non-EVM
    IDepositProcessor(depositProcessor).sendMessageNonEVM{value:msg.value}(stakingTarget,
        stakingIncentive, bridgePayload, transferAmount);
}

[0xA5DF]

The function is named sendTokenWithPayloadToEvm(), EVM is clearly not Solana The README says that Solana is going to be supported, not that the bridge is implemented yet and definitely not that it is via this function.

At the same time, code like this exists in Dispenser:

What's your point? The function isn't implemented yet. How can you report a bug in a code that hasn't been written yet?

[georgiIvanov]

Hi @0xA5DF,

My point is the readme is one of the things that dictates what's expected from the codebase. Other sources are the natspec, docs or additional comments/notes.

For what other reason do we have similar questionnaire for ERC-20 tokens, deployed chains, etc. So that participants check what works / is broken / missing. As far as I can see this still holds water when making decisions about issue validity.

[0xA5DF]

I don't see how the README makes this a valid issue. All you can see from going through the README and the code that sendMessageNonEVM() wasn't implemented yet. This isn't an issue, and definitely not a med one. End of discussion