H-01 MitigationConfirmed

[c4-bot-6]

Lines of code

Vulnerability details

Original Issue

H-01: userTotalStaked invariant will be broken due to vulnerable implementations in release()

Comments

The original issue is state variable userTotalStaked is not updated in the release() flow, causing state accounting conflicts. The impact was user might not be able to fully withdraw their entitled staked amount, or user might not be able to be slashed again if they have been released slash amount before.

Mitigation

PR#8 userTotalStaked variable is now updated in release() with the amountToRelease. This brings userTotalStaked and selfStakes/communityStakes accounting in sync in release() flow.

  function release(
    address staker,
    address stakee,
    uint88 amountToRelease,
    uint16 slashRound
  ) external onlyRole(RELEASER_ROLE) whenNotPaused {
...
    userTotalStaked[staker] += amountToRelease;
    if (staker == stakee) {
...
      selfStakes[staker].slashedAmount -= amountToRelease;
      selfStakes[staker].amount += amountToRelease;
    } else {
...
      communityStakes[staker][stakee].slashedAmount -= amountToRelease;
      communityStakes[staker][stakee].amount += amountToRelease;
...

(https://github.com/gitcoinco/id-staking-v2/blob/7c19717aeab91a0166fc1ca50f443ee2ce7483f0/contracts/IdentityStaking.sol#L665C1-L665C48) In addition, since userTotalStaked, selfStakes/communityStakes are in sync in all other flows (staking, withdrawing and slashing). The issue of accounting conflict caused by userTotalStaked is effectively mitigated.

Test

Attack vectors averted with mitigation. Added tests passed.

  IdentityStaking
    slashing/releasing/burning tests
      ✔ H-01 mitigation success - 'user escape being slashed' averted (39ms)
      ✔ H-01 mitigation success - 'userTotalStaked is broken, user lose funds' averted (38ms)

  2 passing (5s)

Conclusion

LGTM

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as confirmed for report

[GalloDaSballo]

Agree with the considerations

I highly recommend invariant testing on never reverting on that line, perhaps by having a custom error and having a invariant test that ensures the function never reverts with that specific error