#37427 [SC-Critical] Delegation is not updated on slash and unlock

Submitted on Dec 4th 2024 at 14:44:39 UTC by @shadowHunter for Audit Comp | Celo

Report ID: #37427
Report Type: Smart Contract
Report severity: Critical
Target: https://github.com/celo-org/celo-monorepo/blob/release/core-contracts/12/packages/protocol/contracts/governance/LockedGold.sol
Impacts:
- Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results

Description

Brief/Intro

It was observed that delegation is not updated on slashing for reporter and caller in unlock operation. This causes incorrect balance accounting

Vulnerability Details

Observe the 'slash' function and 'unlock` function
Both of this function calls _incrementNonvotingAccountBalance and _decrementNonvotingAccountBalance which changes the non voting balance and thus AccountTotalLockedGold

function getAccountTotalLockedGold(address account) public view returns (uint256) {
    uint256 total = balances[account].nonvoting;
    return total.add(getElection().getTotalVotesByAccount(account));
  }

Lets say User A is unlocking or calling slash on another User B then it is always required to call _updateDelegatedAmount on both Users so that delegated balance gets updated correctly as per new total locked
But the same is missing in both functions

Impact Details

Delegated balance power will be incorrect

References

In unlock call _updateDelegatedAmount(msg.sender); In slash call _updateDelegatedAmount(reporter);

Proof of Concept

function test_ShouldReduceAccountsLockedGoldBalance_WhenAccountIsSlashedForAllOfItsLockedGoldAndIsDelegating()
    public
  {
    uint256 penalty = value;
    uint256 reward = value / 2;
    whenVoteSigner_LockedGoldDelegateGovernanceVotes();
    vm.prank(caller);
    lockedGold.delegateGovernanceVotes(delegatee1, FixidityLib.newFixedFraction(30, 100).unwrap());
    assertEq(lockedGold.getAccountNonvotingLockedGold(caller), 1000);
    assertEq(lockedGold.getAccountTotalLockedGold(caller), 1000);
    assertEq(lockedGold.getAccountTotalGovernanceVotingPower(delegatee1), 300);
    helper_WhenAccountIsSlashedForAllOfItsLockedGold(penalty, reward, delegator);
    assertEq(lockedGold.getAccountNonvotingLockedGold(delegator), 0);
    assertEq(lockedGold.getAccountTotalLockedGold(delegator), 0);
    assertNotEq(lockedGold.getAccountTotalGovernanceVotingPower(delegatee1), 300);
  }