Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Outdated CURRENT_MAX_PRECOMPILE_ADDRESS #961

Closed
c4-submissions opened this issue Oct 23, 2023 · 3 comments
Closed

Outdated CURRENT_MAX_PRECOMPILE_ADDRESS #961

c4-submissions opened this issue Oct 23, 2023 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-888 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/Constants.sol#L35

Vulnerability details

Impact

The Constants contract defines a CURRENT_MAX_PRECOMPILE_ADDRESS constant that should be set to the max precompile address.

address constant ECRECOVER_SYSTEM_CONTRACT = address(0x01);
address constant SHA256_SYSTEM_CONTRACT = address(0x02);
address constant ECADD_SYSTEM_CONTRACT = address(0x06);
address constant ECMUL_SYSTEM_CONTRACT = address(0x07);
 
/// @dev The current maximum deployed precompile address.
/// Note: currently only two precompiles are deployed:
/// 0x01 - ecrecover
/// 0x02 - sha256
/// Important! So the constant should be updated if more precompiles are deployed.
uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));

The max precompile address is ECMUL_SYSTEM_CONTRACT = address(0x07) but it is set to SHA256_SYSTEM_CONTRACT = address(0x02) instead. There is even a comment that it is "Important" to update this value.

/// Important! So the constant should be updated if more precompiles are deployed.

Code that relies on this address to be accurate does not work correctly. For example, the AccountCodeStorage.getCodeHash function should return the EMPTY_STRING_KECCAK = 0xc5... code hash for ECADD_SYSTEM_CONTRACT and ECMUL_SYSTEM_CONTRACT but returns bytes32(0) instead.

address account = address(uint160(_input));
if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS) {
    return EMPTY_STRING_KECCAK;
}

Proof of Concept

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/Constants.sol#L35
https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#93

Tools Used

Manual

Recommended Mitigation Steps

Consider changing this line:

- uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));
+ uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(ECMUL_SYSTEM_CONTRACT));

Assessed type

Error

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 23, 2023
jacobheun pushed a commit that referenced this issue Oct 24, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #142

@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Oct 31, 2023
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 23, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-888 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants