sherlock-audit / 2023-01-optimism-judging

24 stars 10 forks source link

csanuragjain - Steal funds using Portal #260

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

csanuragjain

high

Steal funds using Portal

Summary

It seems you can give arbitrary _data while calling depositTransaction function in OptimismPortal.sol. This means you can spoof a call to l2 bridge with sender as l1 bridge and thus withdraw/steal all funds as shown

Vulnerability Detail

  1. Attacker calls depositTransaction with below args
_to = OTHER_MESSENGER
_value = 0
_gasLimit= _gasLimit
_isCreation=false
_data = abi.encodeWithSelector(
                this.relayMessage.selector,
                messageNonce(),
                L1StandardBridge.sol,
                L2StandardBridge.sol,
                msg.value,
                _minGasLimit,
                abi.encodeWithSelector(finalizeBridgeERC20, WETH,WETH_REMOTE, ANY_ADDR, ATTACKER_ADDR, WETH_AMOUNT, "")
            )
  1. This deposit transaction is recorded and executed

  2. This calls OTHER_MESSENGER contract's relayMessage function with _sender as L1StandardBridge and target as L2StandardBridge (as shown in step 1 payload)

  3. Thus xDomainMsgSender is set to L1StandardBridge

xDomainMsgSender = _sender;
  1. Now L2StandardBridge is called with args (finalizeBridgeERC20, WETH,WETH_REMOTE, ANY_ADDR, ATTACKER_ADDR, WETH_AMOUNT, "")
bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);
  1. This calls finalizeBridgeERC20 function with args (WETH,WETH_REMOTE, ANY_ADDR, ATTACKER_ADDR, WETH_AMOUNT, "") where function args stand for
finalizeBridgeERC20(
        address _localToken,
        address _remoteToken,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _extraData
    )
  1. The onlyOtherBridge requirement fulfill as sender is L1StandardBridge and function executes
  2. This mints the wrapped remote weth to ATTACKER_ADDR with WETH_AMOUNT which is wrong

Impact

Attacker can steal user and contract funds

Code Snippet

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

Tool used

Manual Review

Recommendation

If depositTransaction function is directly called then do not allow _to address to be OTHER_MESSENGER

rcstanciu commented 1 year ago

Comment from Optimism


Description: Steal funds using Portal

Reason: Incorrect idea of spoofing from the L1 Messenger. This will be prevented by the failedMessage mapping in the CrossDomainMessenger.