csanuragjain - Incorrect owner check

[github-actions[bot]]

csanuragjain

low

Incorrect owner check

Summary

The depositTransaction of OptimismPortal can directly be called by user instead of intermediate contract. This means from address wont be aliased. But this is not considered in CrossDomainOwnable contract which plainly undoL1ToL2Alias the caller

Vulnerability Detail

1. Assume depositTransaction is called by User A directly. Since no intermediary contract so no aliasing is done
2. On L2 side, if _checkOwner is checked

function _checkOwner() internal view override {
        require(
            owner() == AddressAliasHelper.undoL1ToL2Alias(msg.sender),
            "CrossDomainOwnable: caller is not the owner"
        );
    }

3. This will try undoL1ToL2Alias on User A address and then match with owner which is incorrect since User A address was never aliased on L1

Impact

The owner check might fail for genuine transaction

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/CrossDomainOwnable.sol#L21

Tool used

Manual Review

Recommendation

This check need to be revised. If the transaction came directly from tx.origin (without any intermediary contract) then no need of removing aliasing

[csanuragjain]

Escalate for 28 USDC

This is mentioning a valid issue where CrossDomainOwnable.sol#L21 is always assuming call to depositTransaction coming from contract (as undo Alias is always done). If depositTransaction is called by a user instead then CrossDomainOwnable will fail to check properly and not allow a genuine user

[sherlock-admin]

Escalate for 28 USDC

This is mentioning a valid issue where CrossDomainOwnable.sol#L21 is always assuming call to depositTransaction coming from contract (as undo Alias is always done). If depositTransaction is called by a user instead then CrossDomainOwnable will fail to check properly and not allow a genuine user

You've created a valid escalation for 28 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation accepted and labeling as specification issue

[sherlock-admin]

Escalation accepted and labeling as specification issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.