Unchecked msg.value will lead to losing funs inside the contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L199

Vulnerability details

Impact

While paying for the transaction,

function payForTransaction(
    bytes32, // _txHash
    bytes32, // _suggestedSignedHash
    Transaction calldata _transaction
) external payable ignoreNonBootloader ignoreInDelegateCall {
    //@audit-issue no checks that actulayy there is msg.value. If that value is not sent, if there 
  are funds in the vootloader, it will be paid from there, causing free transactions

    /**

function payToTheBootloader(Transaction calldata _transaction) internal returns (bool success) {
    address bootloaderAddr = BOOTLOADER_FORMAL_ADDRESS;
    uint256 amount = _transaction.maxFeePerGas * _transaction.gasLimit;

    assembly {
        success := call(gas(), bootloaderAddr, amount, 0, 0, 0, 0)
    }
}
    */
    bool success = _transaction.payToTheBootloader();
    require(success, "Failed to pay the fee to the operator");
}

You can see that on the external call to bootloaderAddr they are sending an amount to pay for the gas, which is intended to come from the payForTransaction function because it is payable. The issue is that they do not check whether the amount was actually sent. Therefore funds already in the contract, will be sent as the amount to the bootloaderAddr

Proof of Concept

Test to confirm that. Add the specific params that you want as the transaction:

 interface PayT{
 function payForTransaction(
    bytes32, // _txHash
    bytes32, // _suggestedSignedHash
    Transaction calldata _transaction
) external;

    struct Transaction {
// The type of the transaction.
uint256 txType;
// The caller.
uint256 from;
// The callee.
uint256 to;
// The gasLimit to pass with the transaction.
// It has the same meaning as Ethereum's gasLimit.
uint256 gasLimit;
// The maximum amount of gas the user is willing to pay for a byte of pubdata.
uint256 gasPerPubdataByteLimit;
// The maximum fee per gas that the user is willing to pay.
// It is akin to EIP1559's maxFeePerGas.
uint256 maxFeePerGas;
// The maximum priority fee per gas that the user is willing to pay.
// It is akin to EIP1559's maxPriorityFeePerGas.
uint256 maxPriorityFeePerGas;
// The transaction's paymaster. If there is no paymaster, it is equal to 0.
uint256 paymaster;
// The nonce of the transaction.
uint256 nonce;
// The value to pass with the transaction.
uint256 value;
// In the future, we might want to add some
// new fields to the struct. The `txData` struct
// is to be passed to account and any changes to its structure
// would mean a breaking change to these accounts. In order to prevent this,
// we should keep some fields as "reserved".
// It is also recommended that their length is fixed, since
// it would allow easier proof integration (in case we will need
// some special circuit for preprocessing transactions).
uint256[4] reserved;
// The transaction's calldata.
bytes data;
// The signature of the transaction.
bytes signature;
// The properly formatted hashes of bytecodes that must be published on L1
// with the inclusion of this transaction. Note, that a bytecode has been published
// before, the user won't pay fees for its republishing.
bytes32[] factoryDeps;
// The input to the paymaster.
bytes paymasterInput;
// Reserved dynamic type for the future use-case. Using it should be avoided,
// But it is still here, just in case we want to enable some additional functionality.
bytes reservedDynamic;
}
}

contract TestPayForTransaction{

 function noPay()external{
    PayT(0xdD870fA1b7C4700F2BD7f44238821C26f7392148).payForTransaction(//custom params. Not send any 
 value to accomplish the attack);
 }

  }

Tools Used

Manual

Recommended Mitigation Steps

Add a check that msg.value >= amount, to avoid that the existing ether in the contract is sent as a result of non having msg.value

[GalloDaSballo]

You can pay from your account balance, this looks off

[miladpiri]

Likely a misunderstanding of how the AA works. The user is supposed to pay the bootloader. Having the function as payable may be a bit confusing, but it is done purely to be more future-compatible (there are no plans to introduce bootloader sending funds to the user while calling this transaction, but just in case it was added there). So it is misunderstanding of how the AA pays for transaction and how the bootloader handle it.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

I think that this may meet the requirements for self-rekt because the sender could send more than the intended amount

That said, the finding was sent as High, and lacks detail, am closing as overly inflated

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity