code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync//blob/main/contracts/libraries/TransactionHelper.sol#L405

Vulnerability details

Impact

A user may provide a non-zero entry for the "_transaction.paymaster" field for a transaction to ensure they do not have to pay the gas fees. However, certain values of "_transaction.paymaster" >= 2^160 can result in address(uint160(_transaction.paymaster) to be equal to zero address, thus, requiring the user to pay the gas as well as the value.

Proof of Concept

The below solidity contract illustrates the bug. The "Test" contract has a state variable "transactionPaymaster" that corresponds to "_transaction.paymaster" in the transaction struct. This state variable has a specific value (2^160).

We can clearly see that the function "check()" in the "Test" contract does not revert, that is, the assert test passes. This shows that the computation address(uint160(transactionPaymaster) results in zero address even when transactionPaymaster is non-zero. This computation is the same as found in the mentioned vulnerable line of the TransactionHelper.sol library.

// SPDX-License-Identifier: GPL-3.0

pragma solidity ^0.8.0;

contract Test {

uint256 public transactionPaymaster = 2**160;

function check() external view {

    assert(address(uint160(transactionPaymaster)) == address(0));
}

}

Tools Used

Remix IDE

Recommended Mitigation Steps

It is recommended to use uint160 instead of uint256 for the paymaster field of a transaction struct. OR It is recommended to check for (_transaction.paymaster != 0) before checking for (address(uint160(_transaction.paymaster)) != address(0)).

GalloDaSballo commented 1 year ago

There is no possibility of a clash because 2^160 is the maximum possible amount of addresses

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-03-zksync-findings