sherlock-audit / 2023-01-optimism-judging

24 stars 10 forks source link

obront - Relayers can send additional gas with cross domain messages #272

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

obront

medium

Relayers can send additional gas with cross domain messages

Summary

When relaying cross domain messages, the relayer has the ability to choose any amount of gas to forward to the call, as long as it exceeds the gasLimit specified by the user. In some cases, this may cause unexpected behavior (for example, when users use gas limits to protect against reentrancy).

Vulnerability Detail

When a user submits a withdrawal transaction, they specify a quantity of gas that they would like to send along with their transaction on the L1 side, called gasLimit.

On L1, gasLimit is used to specify the maximum amount of gas a user is willing to spend, so it would be reasonable for them to assume that this gasLimit also imposes a maximum.

However, when the relayer calls the function to execute their withdrawal transaction, they can send over an arbitrary amount of gas. This gas will all be forwarded along with the execution, regardless of whether it exceeds the gasLimit.

This is how the call happens in OptimismPortal:

bool success = SafeCall.call(
    _tx.target,
    gasleft() - FINALIZE_GAS_BUFFER,
    _tx.value,
    _tx.data
);

Here is the equivalent call in CrossDomainMessenger:

bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);

In both cases, gas sent is determined by gasleft() rather than _tx.gasLimit.

While this usually will not pose any risk to a withdrawer, there are many situations where gas limits are used to achieve specific behavior. Most notably, using .transfer() and .call() rely on gas limits to prevent reentrancy, and it would not be unfair for a user to try to achieve something similar by limiting the gas sent to their contract from the bridge.

The result is that the outcome of the call is partially determined by the relayer instead of the message sender, which is counterintuitive and possibly exploitable.

Impact

Certain withdrawal executions may vary depending on the relayer's choices of the amount of gas to send.

In some situations, if a user is relying on gas limits to protect against reentrancy, an exploiter can finalize the withdrawal on their behalf with more gas, and use this additional gas to perform a reentrancy attack.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324-L329

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L324

Tool used

Manual Review

Recommendation

In order to create a predictable experience, send the exact gas limit specified by the message sender to the recipient:

xDomainMsgSender = _sender;
require(gasleft() >= max(_minGasLimit * 64 / 63, _minGasLimit + RELAY_GAS_BUFFER)
bool success = SafeCall.call(_target, _minGasLimit, _value, _message);
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
rcstanciu commented 1 year ago

Comment from Optimism


Description: Relayers can send additional gas with cross domain messages

Reason: This is correct but deemed a low risk, as it has not been an issue in the legacy system.

Action: We should change the naming of _gasLimit to _minGasLimit to lower confusion. Also, we should consider allowing the option for an exact amount of gas to be forwarded for L2->L1 messages, rather than a minimum amount.