Arbitrum retryable ticket can be used to bypass the tx.origin check

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/helper/ContractWhitelist.sol#L35 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L409

Vulnerability details

Impact

Arbitrum retryable ticket can be used to bypass the tx.origin check

Proof of Concept

There is a function _isEligibleSender

  function _isEligibleSender() internal view {
    // the below condition checks whether the caller is a contract or not
    if (msg.sender != tx.origin)
      require(whitelistedContracts[msg.sender], "Contract must be whitelisted");
  }

as the contract suggest, the code intended to check that if the caller is a EOA account or a smart contract

if it EOA account, the check pass, if the caller is a smart contract, the code check if the smart contract is whitelisted

However, arbitrum has a built-in cross-chain message protocol called retryable tickets

the retryable ticket can let user submit transaction (retryable ticket) in mainnet L1 and then the transaction will get executed in l2

the most important thing is that

https://docs.arbitrum.io/arbos/l1-to-l2-messaging#address-aliasing

All messages submitted via the Delayed Inbox get their sender's addressed "aliased": when these unsigned messages are executed on L2, the sender's address —i.e., that which is returned by msg.sender — will not simply be the L1 address that sent the message; rather it will be the address's "L2 Alias." An address's L2 alias is its value increased by the hex value 0x1111000000000000000000000000000000001111:

The Arbitrum protocol's usage of L2 Aliases for L1-to-L2 messages prevents cross-chain exploits that would otherwise be possible if we simply reused the same L1 addresses as the L2 sender; i.e., tricking an L2 contract that expects a call from a given contract address by sending retryable ticket from the expected contract address on L1.

basically when user submit a transaction in L1 and when the transaction get executed in L2, the msg.sender and tx.origin is aliased

note that another L2 optimism use the same address alias mechanism and the doc summarize very well

https://github.com/ethereum-optimism/optimism/blob/develop/specs/deposits.md#execution

Note for application developers: because CALLER and ORIGIN are set to from, the semantics of using the tx.origin == msg.sender check will not work to determine whether or not a caller is an EOA during a deposit transaction.

If we go to https://www.evmdiff.com/

and we select ethereum with arbitrum and search for opcode

we get

If the transaction is an L1 ⇒ L2 transaction, then tx.origin is set to the aliased address of the address that triggered the L1 ⇒ L2 transaction. Otherwise, this opcode behaves normally.

and

If the transaction is an L1 ⇒ L2 transaction, and this is the initial call (rather than an internal transaction from one contract to another), then msg.sender is set to the aliased address of the address that triggered the L1 ⇒ L2 transaction. Otherwise, this opcode behaves normally.

then basically a smart contract can submit a retryable ticket on L1 and trigger the function that use _isEligibleSender as safeguard that mean to not let smart contract make the call

In particular the function calculateFunding in Atlantic Vault.sol and update the funding amount without admin's permission

Tools Used

Manual Review

Recommended Mitigation Steps

do not use tx.origin to check, only check if the caller is whitelisted or has certain role

Assessed type

Access Control

[c4-pre-sort]

bytes032 marked the issue as primary issue

[bytes032]

Could be related to #583

[c4-sponsor]

psytama (sponsor) disputed

[psytama]

All the main function that a user interacts with to deposit or withdraw does not have this. and we plan on removing it in the admin only functions also.

[GalloDaSballo]

Really interesting vector

[GalloDaSballo]

I feel like the Warden has found a new category of attacks

For the code in scope the finding is a gotcha as no specific impact was demonstrated

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)