Wrong Execution Due to Pay Master Input Overlap in TransactionHelper Contract

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/system-contracts/contracts/libraries/TransactionHelper.sol#L374

Vulnerability details

Impact

Wrong Execution Due to Pay Master Input Overlap in TransactionHelper Contract

Proof of Concept

 function processPaymasterInput(Transaction calldata _transaction) internal {
        require(_transaction.paymasterInput.length >= 4, "The standard paymaster input must be at least 4 bytes long");

>>>        bytes4 paymasterInputSelector = bytes4(_transaction.paymasterInput[0:4]);
        if (paymasterInputSelector == IPaymasterFlow.approvalBased.selector) {
            require(
                _transaction.paymasterInput.length >= 68,
                "The approvalBased paymaster input must be at least 68 bytes long"
            );

            // While the actual data consists of address, uint256 and bytes data,
            // the data is needed only for the paymaster, so we ignore it here for the sake of optimization
>>>            (address token, uint256 minAllowance) = abi.decode(_transaction.paymasterInput[4:68], (address, uint256));
            address paymaster = address(uint160(_transaction.paymaster));

            uint256 currentAllowance = IERC20(token).allowance(address(this), paymaster);
            if (currentAllowance < minAllowance) {
                // Some tokens, e.g. USDT require that the allowance is firsty set to zero
                // and only then updated to the new value.

                IERC20(token).safeApprove(paymaster, 0);
                IERC20(token).safeApprove(paymaster, minAllowance);
            }
        } else if (paymasterInputSelector == IPaymasterFlow.general.selector) {
            // Do nothing. general(bytes) paymaster flow means that the paymaster must interpret these bytes on his own.
        } else {
            revert("Unsupported paymaster flow");
        }
    }

The code above shows how processPaymasterInput(...) is implemented in the TransactionHelper contract, a look at the first pointer from the code above shows that bytes4(_transaction.paymasterInput[0:4] was used to derive paymasterInputSelector, the problem is that later in the code in the second point instead of using _transaction.paymasterInput[5:68] to derive token and minAllowance, the protocol used _transaction.paymasterInput[4:68], i.e [4:68] instead of [5:68], since the data for paymasterInputSelector is between 0 to 4, the value for token and minAllowance should start from 5 not 4.

Tools Used

Manual Review

Recommended Mitigation Steps

Protocol should make necessary adjustments to prevent input value overlap which could end up breaking protocol. The adjustment should be made as provided below

 function processPaymasterInput(Transaction calldata _transaction) internal {
        require(_transaction.paymasterInput.length >= 4, "The standard paymaster input must be at least 4 bytes long");

        bytes4 paymasterInputSelector = bytes4(_transaction.paymasterInput[0:4]);
        if (paymasterInputSelector == IPaymasterFlow.approvalBased.selector) {
            require(
                _transaction.paymasterInput.length >= 68,
                "The approvalBased paymaster input must be at least 68 bytes long"
            );

            // While the actual data consists of address, uint256 and bytes data,
            // the data is needed only for the paymaster, so we ignore it here for the sake of optimization
---            (address token, uint256 minAllowance) = abi.decode(_transaction.paymasterInput[4:68], (address, uint256));
+++            (address token, uint256 minAllowance) = abi.decode(_transaction.paymasterInput[5:68], (address, uint256));
            address paymaster = address(uint160(_transaction.paymaster));

            uint256 currentAllowance = IERC20(token).allowance(address(this), paymaster);
            if (currentAllowance < minAllowance) {
                // Some tokens, e.g. USDT require that the allowance is firsty set to zero
                // and only then updated to the new value.

                IERC20(token).safeApprove(paymaster, 0);
                IERC20(token).safeApprove(paymaster, minAllowance);
            }
        } else if (paymasterInputSelector == IPaymasterFlow.general.selector) {
            // Do nothing. general(bytes) paymaster flow means that the paymaster must interpret these bytes on his own.
        } else {
            revert("Unsupported paymaster flow");
        }
    }

Assessed type

Context

[c4-sponsor]

razzorsec (sponsor) disputed

[razzorsec]

Calldata indexing is exclusive for the end bytes, i.e., [start:end) because index starts from 0. Hence, _transaction.paymasterInput[0:4] will be the 4 bytes selector from 0-3 bytes, and so the _transaction.paymasterInput[4:68] will be the 64 bytes abi.encoded address token, uint256 minAllowance from 4-67 bytes. Hence, we agreed with the issue to be marked invalid

[alex-ppg]

The Warden specifies that there is an overlap in the byte indices utilized by the function, however, as the Sponsor correctly points out, this is incorrect given that the ranges are non-inclusive in the upper end ([start,end) using standard range notation).

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid