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

Governor admin can rug the protocol by proposing and executing malicious proposal by himself after the timelock #167

Closed
code423n4 opened this issue Nov 6, 2022 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-239 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/DiamondCut.sol#L22
https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/DiamondCut.sol#L46

Vulnerability details

Impact

Governor admin can rug the protocol by proposing and executing malicious proposal by himself after the timelock

Proof of Concept

There is nothing stop compromised governor admin rug the protocol by proposing and executing malicious proposal by himself after the timelock.

  1. the admin can create a malicious proposal and call the function proposeDiamondCut
 function proposeDiamondCut(Diamond.FacetCut[] calldata _facetCuts, address _initAddress) external onlyGovernor {

The malicious proposal can be adding malicious facet and steal user fund.
The malicious proposal can be removing all the diamond facet or freeze all facet.

  1. The admin needs to wait UPGRADE_NOTICE_PERIOD, which is 14 days currently beause of the check
bool approvedBySecurityCouncil = s.diamondCutStorage.securityCouncilEmergencyApprovals >=
	SECURITY_COUNCIL_APPROVALS_FOR_EMERGENCY_UPGRADE;

bool upgradeNoticePeriodPassed = block.timestamp >=
	s.diamondCutStorage.proposedDiamondCutTimestamp + UPGRADE_NOTICE_PERIOD;

require(approvedBySecurityCouncil || upgradeNoticePeriodPassed, "a6"); // notice period should expire
require(approvedBySecurityCouncil || !diamondStorage.isFrozen, "f3");
// should not be frozen or should have enough security council approvals
  1. the admin does not need the security council to approve the proposal, after the wait UPGRADE_NOTICE_PERIOD, the admin governor can just call:
   function executeDiamondCutProposal(Diamond.DiamondCutData calldata _diamondCut) external onlyGovernor {

to complete executing the malicious proposal.

  1. Since only the governor has this power, user and other security council can do nothing if the governor misbehave.

Tools Used

Manual Review

Recommended Mitigation Steps

I think having the governor propose a proposal and requesting at least SECURITY_COUNCIL_APPROVALS_FOR_EMERGENCY_UPGRADE approve from the security council

can make the executing new proposal process more decentralized.

@code423n4 code423n4 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 Nov 6, 2022
code423n4 added a commit that referenced this issue Nov 6, 2022
@miladpiri
Copy link

Notice period is there to protect any instant upgrade. Moreover, instant upgrade can be executed only by approval of security council members. Not a valid issue!

@c4-sponsor
Copy link

miladpiri marked the issue as sponsor disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 22, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Dec 2, 2022

GalloDaSballo marked the issue as duplicate of #144

@GalloDaSballo
Copy link

L

@c4-judge c4-judge added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax downgraded by judge Judge downgraded the risk level of this issue duplicate-239 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Dec 2, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Dec 2, 2022

Duplicate of #239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-239 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

6 participants