search

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

6 stars 1 forks source link

Contracts are susceptible for Head Overflow Bug in Calldata #182

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/TransactionHelper.sol#L117-L142 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L77-L87 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/L2EthToken.sol#L97-L99

Vulnerability details

Impact

ABI-encoding a tuple with a statically-sized calldata array in the last component would corrupt 32 leading bytes of its first dynamically encoded component.

Proof of Concept

The functions are taking input arguments as calldata.

Following contracts are using the family of abi.encoding with the data provided input as calldata. TransactionHelper.sol, DefaultAccount.sol, L2EthToken.sol

when we look at the function in TransactionHelper.sol - _encodeHashEIP712Transaction, it takes the stucure of call data as input and does abi.encoding.

As per the solidity page update in the list of known bugs,https://blog.soliditylang.org/2022/08/08/calldata-tuple-reencoding-head-overflow-bug/

The list of criteria where this issue would be possible are,

1.The last component of the tuple is a (potentially nested) statically-sized calldata array with the most base type being either uint or bytes32. E.g. bytes32[10] or uint[2][2][2] 2.The tuple contains at least one dynamic component. E.g. bytes or a struct containing a dynamic array. 3.The code is using ABI coder v2, which is the default since Solidity 0.8.0.

when we look at the Transaction ,

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; } It has the following parameters uint256 , uint256[4], bytes , bytes32[]. These are satisfy the above mentioned three criteria.

Note that structs are represented as tuples in the ABI. Parameter and return value lists of external functions, events and errors are also implicitly treated as tuples.

The bug is the result of overly eager cleanup performed by the compiler when copying calldata arrays to memory during ABI-encoding. Arrays in memory always occupy a multiple of 32 bytes and when the base type does not fill the whole word, the unused space is guaranteed to be zeroed and remain clean after all high-level Solidity operations. In the affected cases the compiler would emit code that incorrectly cleaned the end of an array stored in the last component of a tuple, zeroing 32 bytes belonging to the first dynamic component of the tuple.

When the input for the ABI encoder comes from calldata, large parts of it can simply be copied to the output. The encoder, however, must ensure that the unused spaces between elements are actually cleaned.

for more technical details, refer the link - https://blog.soliditylang.org/2022/08/08/calldata-tuple-reencoding-head-overflow-bug/

Tools Used

Manual review, Solidity language page

Recommended Mitigation Steps

We would suggest to use the latest solidity compiler version.

GalloDaSballo commented 1 year ago

Invalid per this config: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/hardhat.config.ts#L21

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

aktech297 commented 1 year ago

Hi @GalloDaSballo would like to share my views on this issue.

Given the scope of the audit which clearly mentions the contract files alone. https://code4rena.com/contests/2023-03-zksync-era-system-contracts-contest#:~:text=Files%20in%20scope,2418 When we see at the contract files, all are written in solidity version of 0.8.0 kindly refer the few of the files as an example,

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

I have seen in past judging that anything within the scope would be a valid issue. For known issues or assumptions, the team always should shares the details.

Since it is competition, we as wardens should strictly adhere to the set of rules such as known findings which are clearly stated. we are allowed to report any issues even if the team knew before the contest and these are not put on table as known one. These issues are treated as valid findings.

For the issues that deemed to be known, they should be clearly mentioned in the contest's acknowledgement.

I kindly request the judges to take a look and share their thought about the validity of this issue for the given audit scope.

GalloDaSballo commented 1 year ago

Thank you for your thoughts @aktech297

The caret ^ operator is used to enforce the usage of Solidity Compilers in the same Major Version, meaning that if we were to apply your logic blindly, we'd perform the same logical reasoning:

Because we have access to the full repo, as well as deployed contracts, as well as the test suite, it is trivial to verify that the contracts compile using 0.8.16

After re-reviewing the disclosure, the example you brought forward (Transaction struct) doesn't meet the first requirement for the bug (last element being a statically-sized array)

Additionally, the submission you brought forward doesn't show an instance of the bug happening, meaning that from my estimate it doesn't meet the burden of proof requirement

I'm happy to stand corrected, but I will ask of you to: