Incorrect callValueRefundAddress in createRetryableTicket function leads to cross-chain message not delivered

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/staking/ArbitrumDepositProcessorL1.sol#L189-L191

Vulnerability details

Vulnerability Detail

The createRetryableTicket delivers/propagate the Information to the Arbitrum L2 about targets, stakingIncentives. However, incorrect callValueRefundAddress is set, which could result in the information not to be delivered and lost.

According to the Technical documentation we see that during the createRetryableTicket following params should hold:

1. excessFeeRefundAddress: The L2 address to which the excess fee is credited. This is going to be set as the Alias Timelock address on Arbitrum
2. callValueRefundAddress: The L2 address to which the l2CallValue is credited if the ticket times out or gets cancelled. This is going to be set as the Alias Timelock address on Arbitrum

However, both these params are controlled by the user, who initiated the tx. These params are encoded in the bridgePayload and right before cross-chain transfer they are decoded to be passed into the the createRetryableTicket function. Let’s take a look

// Decode the staking contract supplemental payload required for bridging tokens
        (address refundAccount, uint256 gasPriceBid, uint256 maxSubmissionCostToken, uint256 gasLimitMessage,
            uint256 maxSubmissionCostMessage) = abi.decode(bridgePayload, (address, uint256, uint256, uint256, uint256));

        // If refundAccount is zero, default to msg.sender
        if (refundAccount == address(0)) {
            refundAccount = msg.sender;
        }

So, from the bridgePayload, the refundAccount could be any address, if it is set to address(0) , it is set to msg.sender.

According to the Arbitrum docs, the callValueRefundAddress (which is set to any arbitrary address in our case) got a critical permission to cancel the ticket.

Proof of Concept

1. Malicious user calls claimStakingIncentives fn in the Dispenser.sol with malicious address encoded into the bridgePayload parameter.

2. calculation of staking incentives happens + Refund returned amount back to tokenomics inflation

3. The internal fn _distributeStakingIncentives is called which will execute the cross-chain tx.

4. Following the tx flow we reach the ArbitrumDepositProcessorL1, the _sendMessage function. At this step the bridgePayload is decoded with malicious refundAccount

5. createRetryableTicket function doesn’t differentiate between excessFeeRefundAddress and callValueRefundAddress and simply use refundAccount for both.

   sequence = IBridge(l1MessageRelayer).createRetryableTicket{value: cost[1]}(l2TargetDispenser, 0,
               maxSubmissionCostMessage, refundAccount, refundAccount, gasLimitMessage, gasPriceBid, data);

6. Once the ticket is created, the malicious “refundAccount” manually cancel the ticket and on Arbitrum the Ticket is deleted event is emmited. It means that receiveMessage function will never be called on the L2.

Impact

On the Arbitrum L2, the receiveMessage won’t be called. And crucial synchronisation step will not be executed. Precisely, _processData function will not be triggered, which is crucial step to sync the withheldAmount which “failed to be delivered” on L2. And when the syncWithheldTokens will be called on L2 (to propagate the withheldAmount on L1) , it will revert if withheldAmount would be 0, or proceed with incorrect params.

Eventually, the correct synchronisation step of withheldAmount could be failed, due to ticket cancellation.

Additionally, If a ticket with a callvalue is eventually discarded (cancelled or expired), having never successfully run, the escrowed callvalue will be paid out to a callValueRefundAddress account that was specified in the initial submission, so the cost of such attack is almost miserable

Recommendation

Ensure, that callValueRefundAddress is set to trusted address, as it is stated in the technical documentation.

Assessed type

Context

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)

[0xA5DF]

Downgrading to med, given that there's the processDataMaintenance() function the impact is limited