Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Files

Latest commit

 

History

History
61 lines (39 loc) · 4.36 KB

069.md

File metadata and controls

61 lines (39 loc) · 4.36 KB

tnch

medium

Incorrect de-aliasing of EOAs in CrossDomainOwnable can lead to contract lockup

Summary

Vulnerability Detail

The CrossDomainOwnable contract intends to provide a building block for cross-domain ownership of contracts. That means that contracts on L2 can be owned by accounts on L1. An owner account on L1 can make permissioned calls to their L2 contracts by calling the depositTransaction function of the OptimismPortal contract on L1. When the owner account on L1 is an EOA, the from address for the call is not aliased to an L2 address.

However, the L2 code in the CrossDomainOwnable always de-aliases the msg.sender account, to then check whether it matches the configured owner account. Even when the caller account on L1 is an EOA that hasn't been aliased by the portal. Therefore, EOAs expected to own L2 contracts that use the CrossDomainOwnable contract will not be able to use them, effectively locking up any permissioned functionality on the owned L2 contract.

Note that while there are tests for the CrossDomainOwnable contract, the issue is missed by an oversight there. In the test, when the L1 EOA ("alice") is pranked with vm.prank(alice), the test is only changing the msg.sender to the alice account for the next call to depositTransaction. But it's not changing the tx.origin to the alice account. To do that, it would need to use vm.prank(alice, alice) (see Forge's docs). Therefore, the portal contract sees that msg.sender does not match tx.origin, aliases the sender, and when the alice account calls the L2 contract, the de-alias makes the test pass.

Here's the fixed version of the test that surfaces the actual bug in the CrossDomainOwnable contract. You can add the function below in the CrossDomainOwnable.t.sol file, under the existing CrossDomainOwnableThroughPortal_Test contract:

contract CrossDomainOwnableThroughPortal_Test is Portal_Initializer {

   // [...]
   
   function test_depositTransaction_crossDomainOwner_origin_fails() external {
        vm.recordLogs();

        vm.prank(alice, alice); // --> Correctly setting the `tx.origin` to the `alice` account
        op.depositTransaction(
            address(setter),
            0,
            10000,
            false,
            abi.encodeWithSelector(XDomainSetter.set.selector, 1)
        );

        VmSafe.Log[] memory logs = vm.getRecordedLogs();
        VmSafe.Log memory log = logs[0];
        address from = Bytes32AddressLib.fromLast20Bytes(log.topics[1]);

        vm.prank(from);
        setter.set(1); // --> Fails with "caller is not the owner"
        assertEq(setter.value(), 1);
    }
}

Impact

EOAs on L1 that own L2 contracts inheriting CrossDomainOwnable will not be able to interact with permissioned functions. There are no contracts in the code base using it at the moment. The contract seems to just be provided as a building block for other devs building on Optimism. So for now the impact is limited to a hypothetical scenario.

Code Snippet

Tool used

Manual review and Foundry for testing.

Recommendation

The CrossDomainOwnable contract doesn't use the L2CrossDomainMessenger contract. So there doesn't seem to be a straightforward way for the CrossDomainOwnable to determine whether the msg.sender must be de-aliased or not before checking permissions.

It seems the most straightforward solution is to warn EOAs to avoid using the CrossDomainOwnable, and instead use the CrossDomainOwnable2 contract. I'd also suggest adding more comprehensive tests to these contracts, explicitly checking for different scenarios where L1 accounts are EOAs or smart contracts.