QA-01 MitigationConfirmed

[c4-bot-10]

Lines of code

Vulnerability details

Original Issue

QA-01: 90 day minimum appeal period can be violated by lockAndBurn in the edge case of protocol pausing

Comments

The original issue points out an edge case of protocol pausing that leads to a violation of 90-day minimum appeal period. When paused, no release() of slashed amounts can be processed due to whenNotPaused modifier.

However, the duration of protocol pausing, will still be counted towards 90-day minimal appeal period. This is because as soon as protocol is unpaused, permissionless lockAndBurn() can be invoked by anyone and the checking of the current round end time will not factor in the duration of pausing. The slash amount will be burned.

In this case, a user might lose the released amount even though approved for a release because technically less than a 90-day window was allocated for the release() flow.

Mitigations

PR#9 The mitigation is making lockAndBurn() access-controlled by BURNER_ROLE. Making lockAndBurn() permissioned allows the timing of lockAndBurn() call to be controlled by the protocol.

  function lockAndBurn() external onlyRole(BURNER_ROLE) whenNotPaused {
    if (block.timestamp - lastBurnTimestamp < burnRoundMinimumDuration) {
      revert MinimumBurnRoundDurationNotMet();
    }
...

(https://github.com/gitcoinco/id-staking-v2/blob/7c19717aeab91a0166fc1ca50f443ee2ce7483f0/contracts/IdentityStaking.sol#L620)

Back to the edge case, when the protocol is paused within a given round. BURNER_ROLE can choose to call lockAndBurn() at a delayed time (after burnRoundMinimumDuration + lastBurnTimestamp) to compensate for the duration of protocol pausing where release() is disabled.

Suppose the protocol was paused for 2h in the current round, BURNER_ROLE can choose to call lockAndBurn() at a 2h delay, allowing a full 90-day minimum appeal time for release().

The mitigation solves the original issue with minimal code change.

Suggestions

It should be noted that the mitigation also comes with increased centralization risk.

It sacrifies the permissionless nature of the maintenance lockAndBurn() call. A permissionless maintenance call is often used by protocols to engage users and provide additional incentives. In addition, reliance on protocol EOAs for maintenance mechanisms might also put the protocol at risk if EOAs are compromised.

If lockAndBurn() was intended to be permissionless for future incentive integration, an alternative mitigation could be simply adding the duration of pausing when checking block.timestamp.

But in any case, current mitigation works without increasing the complexity.

Conclusion

LGTM

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as confirmed for report

[GalloDaSballo]

Agree with centralization risk, but also with consistent timing

[liveactionllama]

Updating the MR- label here - just for consistency across submissions.