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

Risk of Refund Loss During Upgrade Transactions #880

Open
c4-submissions opened this issue Oct 23, 2023 · 5 comments
Open

Risk of Refund Loss During Upgrade Transactions #880

c4-submissions opened this issue Oct 23, 2023 · 5 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) downgraded by judge Judge downgraded the risk level of this issue grade-a ineligible for award QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons 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/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L55

Vulnerability details

Impact

The absence of a mechanism to verify the control of the refund recipient may expose upgrade transactions to the risk of refund loss

Proof of Concept

In the case of an upgrade transaction, there is a risk of losing the refund because there is no mechanism in place to ensure that the refund recipient address is under the control of the governor or administrator.
https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol#L181
https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L55

Since the msg.sender for upgrade transactions is the address of the Governance contract, it is essential that the refund recipient corresponds to the L1toL2Alias of the Governance contract to ensure proper control and governance over the refund.

Tools Used

Recommended Mitigation Steps

The check on line 55 should be as follows:

require(_transaction.reserved[1] == applyL1ToL2Alias(msg.sender), "uf"); 

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L55

Assessed type

Context

@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

141345 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 Nov 2, 2023
@miladpiri
Copy link

More of a process issue than system contracts bug. So, QA.

@c4-sponsor
Copy link

miladpiri marked the issue as disagree with severity

@c4-sponsor c4-sponsor added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Nov 6, 2023
@c4-sponsor
Copy link

miladpiri (sponsor) acknowledged

@c4-sponsor c4-sponsor added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Nov 6, 2023
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 25, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to QA (Quality Assurance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) downgraded by judge Judge downgraded the risk level of this issue grade-a ineligible for award QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

7 participants