Insufficient Minimum Delay Enforcement in `updateDelay` Function

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/governance/Governance.sol#L249-L252

Vulnerability details

Impact

No enforcement of a minimum delay in the updateDelay function could undermine the entire governance mechanism of the contract and lead to the following consequences:

1. Bypassing the intended timelock mechanism, allowing immediate execution of potentially malicious operations without oversight or review.
2. Disrupting contract upgrades by executing unauthorized or malicious upgrades to the governed contracts.
3. Exploiting the execute and executeInstant functions to perform unauthorized actions, such as draining funds, modifying contract state, or executing arbitrary code.
4. Compromising the governance process by nullifying the intended oversight and security measures provided by the timelock.

Proof of Concept

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/governance/Governance.sol#L249-L252

The updateDelay function in the contract lacks enforcement of a minimum delay value when updating the minDelay parameter. This could allow an attacker who gains control of the contract by hook or crook to set the minDelay to zero or an extremely low value, effectively bypassing the intended timelock mechanism for governance operations.

function updateDelay(uint256 _newDelay) external onlySelf {
    emit ChangeMinDelay(minDelay, _newDelay);
    minDelay = _newDelay; // No minimum delay check
}

With a minimal or no delay, an attacker could then schedule and execute malicious operations almost immediately, exploiting functions like scheduleTransparent, scheduleShadow, execute, and executeInstant.

function scheduleTransparent(Operation calldata _operation, uint256 _delay) external onlyOwner {
    bytes32 id = hashOperation(_operation);
    _schedule(id, _delay); // _delay could be set to zero
    emit TransparentOperationScheduled(id, _delay, _operation);
}

function scheduleShadow(bytes32 _id, uint256 _delay) external onlyOwner {
    _schedule(_id, _delay); // _delay could be set to zero
    emit ShadowOperationScheduled(_id, _delay);
}

function execute(Operation calldata _operation) external payable onlyOwnerOrSecurityCouncil {
    // ...
    // With minDelay set to zero, this function could execute immediately
}

function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil {
    // ...
    // With minDelay set to zero, this function could execute immediately
}

Tools Used

God & Manual Review

Recommended Mitigation Steps

I'd recommend enforcing a reasonable minimum delay in the updateDelay function. This can be achieved by adding a check to ensure that the new delay value is greater than or equal to a predefined minimum delay constant.

uint256 public constant MINIMUM_DELAY = 3 days; // Example: 3 days as the minimum delay

function updateDelay(uint256 _newDelay) external onlySelf {
    require(_newDelay >= MINIMUM_DELAY, "New delay should be greater than or equal to the minimum delay");
    emit ChangeMinDelay(minDelay, _newDelay);
    minDelay = _newDelay;
}

Alternatively, the contract could enforce a hard-coded minimum delay that cannot be changed, ensuring a reasonable timelock is always in place.

uint256 public immutable MINIMUM_DELAY = 3 days; // Example: 3 days as the minimum delay

function updateDelay(uint256 _newDelay) external onlySelf {
    require(_newDelay >= MINIMUM_DELAY, "New delay should be greater than or equal to the minimum delay");
    emit ChangeMinDelay(minDelay, _newDelay);
    minDelay = _newDelay;
}

This will ensure that a minimum delay is always enforced, protecting the intended governance process and mitigating the risk of immediate execution of malicious operations.

Assessed type

Access Control

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

We don't think this is an issue. To bypass delay, one needs to wait for the delay so all good.

[alex-ppg]

The Warden describes that the delay configuration of the Governance module is not sanitized.

As the Sponsor describes, an update of the delay would still have to honor the originally configured delay thereby offering adequate time for governance members to react to a malicious change. I do not consider sanitization of the delay a valid medium-risk vulnerability, and such a concern is better submitted in a QA report.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity