the blobGas is been refunded twice in `addSequencerL2BatchFromBlobs` functions

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L508

Vulnerability details

The sequencer contract is been refunding the BatchPoster for gas used calling the function addSequencerL2BatchFromBlobs see the refundsGas modifier:

 modifier refundsGas(IGasRefunder gasRefunder, IReader4844 reader4844) {
        uint256 startGasLeft = gasleft(); 
        _;
        if (address(gasRefunder) != address(0)) {
            uint256 calldataSize = msg.data.length;
            uint256 calldataWords = (calldataSize + 31) / 32;
            // account for the CALLDATACOPY cost of the proxy contract, including the memory expansion cost
            startGasLeft += calldataWords * 6 + (calldataWords**2) / 512; 

            if (msg.sender != tx.origin) { 

            } else {

                if (address(reader4844) != address(0)) {

                    try reader4844.getDataHashes() returns (bytes32[] memory dataHashes) {  <-----
                        if (dataHashes.length != 0) {
                            uint256 blobBasefee = reader4844.getBlobBaseFee();
                            startGasLeft +=
                                (dataHashes.length * gasPerBlob * blobBasefee) /
                                block.basefee;
                        }
                    } catch {} 
                }
            }

            gasRefunder.onGasSpent(payable(msg.sender), startGasLeft - gasleft(), calldataSize);
        }
    }

if the reader4844 is set it, the modifier is refunding the blob gas too see the arrow above.

The problem is that the blob gas fee is been refunded in the addSequencerL2BatchFromBlobsImpl function:

 function addSequencerL2BatchFromBlobs(
        uint256 sequenceNumber,
        uint256 afterDelayedMessagesRead,
        IGasRefunder gasRefunder,
        uint256 prevMessageCount,
        uint256 newMessageCount
    ) external refundsGas(gasRefunder, reader4844) {
        if (!isBatchPoster[msg.sender]) revert NotBatchPoster();
        if (isDelayProofRequired(afterDelayedMessagesRead)) revert DelayProofRequired();

        addSequencerL2BatchFromBlobsImpl(
            sequenceNumber,
            afterDelayedMessagesRead,
            prevMessageCount,
            newMessageCount
        );  <------
    }

[Link]

Impact

the blobGas is been refunded twice in addSequencerL2BatchFromBlobs functions making the protocol loss funds.

Proof of Concept

The BatchPoster is been refunded first in addSequencerL2BatchFromBlobsImpl function (see arrow below):

 function addSequencerL2BatchFromBlobsImpl(
        uint256 sequenceNumber,
        uint256 afterDelayedMessagesRead,
        uint256 prevMessageCount,
        uint256 newMessageCount
    ) internal {
        (
            bytes32 dataHash,
            IBridge.TimeBounds memory timeBounds,
            uint256 blobGas
        ) = formBlobDataHash(afterDelayedMessagesRead);

        (
            uint256 seqMessageIndex,
            bytes32 beforeAcc,
            bytes32 delayedAcc,
            bytes32 afterAcc
        ) = addSequencerL2BatchImpl(
                dataHash,
                afterDelayedMessagesRead,
                0,
                prevMessageCount,
                newMessageCount
            );

        if (seqMessageIndex != sequenceNumber && sequenceNumber != ~uint256(0)) {
            revert BadSequencerNumber(seqMessageIndex, sequenceNumber);
        }

        emit SequencerBatchDelivered(
            sequenceNumber,
            beforeAcc,
            afterAcc,
            delayedAcc,
            totalDelayedMessagesRead,
            timeBounds,
            IBridge.BatchDataLocation.Blob
        );

        if (hostChainIsArbitrum) revert DataBlobsNotSupported();

        if (msg.sender == tx.origin && !isUsingFeeToken) {
            submitBatchSpendingReport(dataHash, seqMessageIndex, block.basefee, blobGas); <-----
        }
    }

[Link]

And second in refundsGas modifier (see the arrow below):

 modifier refundsGas(IGasRefunder gasRefunder, IReader4844 reader4844) {
        uint256 startGasLeft = gasleft(); 
        _;
        if (address(gasRefunder) != address(0)) {
            uint256 calldataSize = msg.data.length;
            uint256 calldataWords = (calldataSize + 31) / 32;
            // account for the CALLDATACOPY cost of the proxy contract, including the memory expansion cost
            startGasLeft += calldataWords * 6 + (calldataWords**2) / 512; 
            // if triggered in a contract call, the spender may be overrefunded by appending dummy data to the call
            // so we check if it is a top level call, which would mean the sender paid calldata as part of tx.input
            // solhint-disable-next-line avoid-tx-origin
            if (msg.sender != tx.origin) { 
                // We can't be sure if this calldata came from the top level tx,
                // so to be safe we tell the gas refunder there was no calldata.
                calldataSize = 0;
            } else {
                // for similar reasons to above we only refund blob gas when the tx.origin is the msg.sender
                // this avoids the caller being able to send blobs to other contracts and still get refunded here
                if (address(reader4844) != address(0)) {
                    // add any cost for 4844 data, the data hash reader throws an error prior to 4844 being activated
                    // we do this addition here rather in the GasRefunder so that we can check the msg.sender is the tx.origin
                    try reader4844.getDataHashes() returns (bytes32[] memory dataHashes) {  <-----
                        if (dataHashes.length != 0) {
                            uint256 blobBasefee = reader4844.getBlobBaseFee();
                            startGasLeft +=
                                (dataHashes.length * gasPerBlob * blobBasefee) /
                                block.basefee;
                        }
                    } catch {} 
                }
            }

            gasRefunder.onGasSpent(payable(msg.sender), startGasLeft - gasleft(), calldataSize);
        }
    }

Tools Used

Manual.

Recommended Mitigation Steps

Consider don't refund the BatchPoster in the addSequencerL2BatchFromBlobsImpl function since the blob gas is been refunding in the refundsGas modifier.

Assessed type

Other

[c4-sponsor]

gzeoneth (sponsor) disputed

[gzeoneth]

Out-of-scope, also invalid. Batch posting report is the protocol paying the batch poster, where gas refunder is used for the batch poster to pay itself from a cold wallet.

[Picodes]

Indeed the same behavior was there in https://github.com/OffchainLabs/nitro-contracts/blob/77ee9de042de225fab560096f7624f3d13bd12cb/src/bridge/SequencerInbox.sol

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope