Era
ZKsync
FoundryLayer 2
500,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Lack of proposal expiry mechanisms can lead to governance exploits.

0xgenaudits

Summary

Approved proposals could remain in a Ready state indefinitely, and malicious user can take advantage of this and execute the proposal under different conditions than the one under which the proposal was submitted.

Vulnerability Details

After security Council or Guardian approves the proposal a delay will starts example in security Council case upgradeStatus[id].securityCouncilApprovalTimestamp = uint48(block.timestamp)
a timestamp will be set to securityCouncilApprovalTimestamp and when this delay passes the proposal will be ready to be executed now the proposal creator will be able to execute the proposal.

The problem occurs when the executor does not call ProtocolUpgradeHandler::execute in this case the proposal will be in Ready state for ever without a way to make it expire.

  1. The approveUpgradeSecurityCouncil function sets the securityCouncilApprovalTimestamp for a proposal when it is approved.

  2. The upgradeState function determines the state of the proposal as follows:

  3. However, if the execute function is not called, the proposal remains in a Ready state forever, as there is no expiry mechanism to transition unexecuted proposals to an Done state.

POC:

Approval Logic

function approveUpgradeSecurityCouncil(bytes32 _id) external onlySecurityCouncil {
UpgradeState upgState = upgradeState(_id);
require(upgState == UpgradeState.Waiting, "Upgrade with this id is not waiting for the approval from Security Council");
​
@>>> // security council approves the proposal.
@>>> upgradeStatus[_id].securityCouncilApprovalTimestamp = uint48(block.timestamp);
emit UpgradeApprovedBySecurityCouncil(_id);
}

State Determination:

After block.timestamp >= upg.securityCouncilApprovalTimestamp + UPGRADE_DELAY_PERIOD;

function upgradeState(bytes32 _id) public view returns(UpgradeState) {
//...
@>>> if (upg.securityCouncilApprovalTimestamp != 0) {
uint256 readyWithSecurityCouncilTimestamp = upg.securityCouncilApprovalTimestamp + UPGRADE_DELAY_PERIOD;
@>>> // timestamp is greater than readyWithSecurityCouncilTimestamp so it will
@>>> // return Ready state.
return block.timestamp >= readyWithSecurityCouncilTimestamp
@>>> ? UpgradeState.Ready
: UpgradeState.ExecutionPending;
}
//...
}

Execution Logic

function execute(UpgradeProposal calldata _proposal) external payable {
bytes32 id = keccak256(abi.encode(_proposal));
UpgradeState upgState = upgradeState(id);
require(upgState == UpgradeState.Ready, "Upgrade is not yet ready");
@>>> // only _proposal.executor can execute the proposal so the proposal
@>>> // will be in Ready state and _proposal.executor can execute this when ever he wants.
require(
_proposal.executor == address(0) || _proposal.executor == msg.sender,
"msg.sender is not authorized to perform the upgrade"
);
​
upgradeStatus[id].executed = true;
_execute(_proposal.calls);
}

Impact

Proposals approved by the Security Council but not executed will remain in an indefinite "Ready" state.

  • This could lead to:

    • Stale proposals lingering in the system.

    • Malicious actor could propose an upgrade knowing he won't execute it now but after specific conditions are meet and leaving it in a "Ready" state. When specific conditions align (e.g: changes in the protocol), the actor could execute the proposal to exploit the system.

Recommendations

Modify the upgradeState function to include a new "Expired" state:

File: https://github.com/Cyfrin/2024-10-zksync/blob/main/zk-governance/l1-contracts/src/ProtocolUpgradeHandler.sol

+ uint256 internal constant MAX_PENDING_PERIOD = 60 days;
function upgradeState(bytes32 _id) public view returns(UpgradeState) {
if (upg.securityCouncilApprovalTimestamp != 0) {
uint256 readyWithSecurityCouncilTimestamp = upg.securityCouncilApprovalTimestamp + UPGRADE_DELAY_PERIOD;
+ if (block.timestamp >= readyWithSecurityCouncilTimestamp + MAX_PENDING_PERIOD) {
+ return UpgradeState.Expired;
+ }
return block.timestamp >= readyWithSecurityCouncilTimestamp
? UpgradeState.Ready
: UpgradeState.ExecutionPending;
}
uint256 waitOrExpiryTimestamp = upg.creationTimestamp + legalVetoTime + UPGRADE_WAIT_OR_EXPIRE_PERIOD;
if (block.timestamp >= waitOrExpiryTimestamp) {
+ if (!upg.guardiansApproval || block.timestamp >= waitOrExpiryTimestamp + MAX_PENDING_PERIOD) {
return UpgradeState.Expired;
}
​
uint256 readyWithGuardiansTimestamp = waitOrExpiryTimestamp + UPGRADE_DELAY_PERIOD;
return block.timestamp >= readyWithGuardiansTimestamp ? UpgradeState.Ready : UpgradeState.ExecutionPending;
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
6 months ago
inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[INVALID] Informational

Admin input validation, gas, missing events not related to bridges, NATSPEC, spellcheck, Address Zero, Indexed fields in Events, 0 impact, trusted admin/party action https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Appeal created

0xgenaudits Submitter
5 months ago
0xgenaudits Submitter
5 months ago
inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Lack of proposal expiry mechanisms can lead to governance exploits.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.