tx.origin may be removed in future and its usage for contract check is not recommended

[code423n4]

Lines of code

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L465

Vulnerability details

Impact

There is a chance that tx.origin will be removed from the Ethereum protocol in the future, so code that uses tx.origin for authentication must be avoid using it.

There is also some EIPs being proposed for change/remove of tx.origin.

ethereum/EIPs#637

https://www.reddit.com/r/ethereum/comments/6d11lv/erc_about_txorigin_change_for_account_abstraction/

In OptimismPortal.sol,

File: contracts/L1/OptimismPortal.sol

464        address from = msg.sender;
465        if (msg.sender != tx.origin) {
466            from = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
467        }

Here, it checks for contract adress by checking msg.sender != tx.origin. These can not work in future if the new EIPs concerning to remove tx.origin removal due to security issues got approved and tx.origin is removed from Ethereum protocol.

Refer this openzeppelin discussion for more information

Proof of Concept

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L465

Tools Used

Manual review

Recommended Mitigation Steps

Change the code as below,

        address from = msg.sender;
-        if (msg.sender != tx.origin) {
+        if (msg.sender.code.length > 0) {
            from = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
        }

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as selected for report

[anupsv]

EIP still to be implemented.

[c4-sponsor]

anupsv marked the issue as sponsor disputed

[0xleastwood]

Treating this as low risk because the contract is upgradeable and assumes that this EIP will be implemented in the near future.

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

0xleastwood marked the issue as not selected for report