#37285 [SC-Critical] Incorrect Delegation State After Slashing in LockedGold Contract

Submitted on Dec 1st 2024 at 16:37:37 UTC by @jovi for Audit Comp | Celo

• Report ID: #37285

• 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

The LockedGold contract fails to properly update the delegated voting power when an account is slashed. Specifically, the contract neglects to call key functions like _updateDelegatedAmount and revokeFromDelegatedWhenUnlocking for the slashed account. This leads to an inconsistent state where delegatees retain voting power based on the pre-slashing balance, causing discrepancies in governance integrity.

Vulnerability Details

Context

The LockedGold contract is responsible for managing locked CELO and delegation of governance votes. To ensure the accuracy of voting power, it employs two key functions:

1. _updateDelegatedAmount:

   • Purpose: Recalculates the CELO delegated to each delegatee based on the current total locked balance of the delegator.

   • Usage: Invoked after any operation that modifies the total locked gold (e.g., lock or relock).

   • Importance: Ensures that delegated voting power is proportionally updated to reflect the delegator’s current locked gold.

2. revokeFromDelegatedWhenUnlocking:

   • Purpose: Adjusts delegation amounts when gold is being unlocked, ensuring that delegatees do not retain voting power over funds no longer locked.

   • Usage: Invoked during the unlocking process before the balance is reduced.

   • Importance: Prevents delegatees from holding inflated voting power after funds are unlocked.

These functions are systematically invoked during standard balance-altering operations such as:

• lock(): Updates delegated amounts after locking additional CELO.

• unlock(): Revokes delegations for the unlocked amount.

• relock(): Recalculates delegations after relocking previously unlocked gold.

However, the slash() function fails to invoke these critical updates.

Vulnerability

When an account is slashed using the slash() function, the contract reduces the account’s locked balance but does not update the associated delegated amounts. This creates a discrepancy between the account's actual locked balance and the delegatees' voting power.

Key issues:

1. Delegation Discrepancy: The slashed account’s delegatees retain voting power based on the pre-slashed balance.

2. Reporter Delegation State: The slasher’s account does not properly reflect delegation power from the reward.

3. Incorrect Temporary State: While the updateDelegatedAmount() function is publicly available for permissionless updates, the temporary inconsistency can be exploited.

The following snippet illustrates the issue:

function slash(
    address account,
    uint256 penalty,
    address reporter,
    uint256 reward,
    address[] calldata lessers,
    address[] calldata greaters,
    uint256[] calldata indices
  ) external onlySlasher {
...
_decrementNonvotingAccountBalance(account, maxSlash.sub(difference));
_incrementNonvotingAccountBalance(reporter, reward);
...
}

This discrepancy creates an inconsistent state in the delegation system, where the actual locked gold doesn't match the delegated amounts temporarily.

Even though there is the updateDelegatedAmount public function that allows permissionless updates of delegated amounts, allowing this explicit transition to a wrong state during slashes may be utilized by exploits to falsely display states that have more voting power than they should.

Impact Details

1. Governance Integrity: Delegatees retain incorrect voting power after slashes, undermining the fairness and accuracy of governance decisions.

2. Economic Imbalance:

   • Slashed accounts effectively retain an unfair delegation advantage.

   • Slasher delegatees may not gain immediate delegation power, leading to delayed or inaccurate representation.

Proof of Concept

Proof of Concept

The following test demonstrates the vulnerability. It creates a delegator account, locks CELO, delegates voting power, and then slashes the account. The test shows that the delegatee retains voting power based on the pre-slashed balance.

Test Code

To demonstrate this vulnerability paste the following modified code snippet at the LockedGold.t.sol test file:

contract LockedGoldTest_slash is LockedGoldTest {
  string slasherName = "DowntimeSlasher";
  uint256 value = 1000;
  address group = actor("group");
  address groupMember = actor("groupMember");
  address reporter = actor("reporter");
  address downtimeSlasher = actor(slasherName);
  address delegatee = actor("delegatee");

  Election electionSlashTest;

  address delegatee1 = actor("delegatee1");
  address delegatee2 = actor("delegatee2");
  address delegatee3 = actor("delegatee3");
  address delegator = actor("delegator");
  address delegator2 = actor("delegator2");

  address delegatorSigner;
  uint256 delegatorSignerPK;
  address delegatorSigner2;
  uint256 delegatorSigner2PK;
  address delegateeSigner1;
  uint256 delegateeSigner1PK;
  address delegateeSigner2;
  uint256 delegateeSigner2PK;

  uint256 percentToDelegate = 30;
  uint256 delegatedAmount = (value * percentToDelegate) / 100;

  uint256 percentToDelegate1 = 30;
  uint256 percentToDelegate2 = 20;
  uint256 percentToDelegate3 = 50;
  uint256 delegatedAmount1 = (value * percentToDelegate1) / 100;
  uint256 delegatedAmount2 = (value * percentToDelegate2) / 100;
  uint256 delegatedAmount3 = (value * percentToDelegate3) / 100;

  function setUp() public {
    super.setUp();
    electionSlashTest = new Election(true);
    registry.setAddressFor("Election", address(electionSlashTest));
    electionSlashTest.initialize(
      address(registry),
      4,
      6,
      3,
      FixidityLib.newFixedFraction(1, 100).unwrap()
    );

    address[] memory members = new address[](1);
    members[0] = groupMember;

    validators.setMembers(group, members);
    registry.setAddressFor("Validators", caller);
    electionSlashTest.markGroupEligible(group, address(0), address(0));
    registry.setAddressFor("Validators", address(validators));
    validators.setNumRegisteredValidators(1);

    lockedGold.lock.value(value)();
    registry.setAddressFor(slasherName, downtimeSlasher);
    lockedGold.addSlasher(slasherName);

    vm.prank(reporter);
    accounts.createAccount();

    
    vm.prank(delegatee1);
    accounts.createAccount();
    vm.prank(delegatee2);
    accounts.createAccount();
    vm.prank(delegatee3);
    accounts.createAccount();
    vm.prank(delegator);
    accounts.createAccount();
    vm.prank(delegator2);
    accounts.createAccount();

    (delegatorSigner, delegatorSignerPK) = actorWithPK("delegatorSigner");
    (delegatorSigner2, delegatorSigner2PK) = actorWithPK("delegatorSigner2");
    (delegateeSigner1, delegateeSigner1PK) = actorWithPK("delegateeSigner1");
    (delegateeSigner2, delegateeSigner2PK) = actorWithPK("delegateeSigner2");

    vm.deal(delegator, 10 ether);
    vm.deal(delegator2, 10 ether);
  }

  function whenVoteSigner_LockedGoldDelegateGovernanceVotes() public {
    helper_WhenVoteSigners(
      WhenVoteSignerStruct(
        delegator,
        delegator2,
        delegatee1,
        delegatee2,
        delegatorSignerPK,
        delegateeSigner1PK,
        delegatorSigner2PK,
        delegateeSigner2PK,
        true
      )
    );
  }

function helper_WhenAccountIsSlashedForAllOfItsLockedGold(
  uint256 penalty,
  uint256 reward
) public {
  address[] memory lessers = new address[](1);
  lessers[0] = address(0);
  address[] memory greaters = new address[](1);
  greaters[0] = address(0);

  uint256[] memory indices = new uint256[](1);
  indices[0] = 0;

  vm.prank(downtimeSlasher);
  lockedGold.slash(delegator, penalty, reporter, reward, lessers, greaters, indices);
}

   function test_POC_ShouldReduceAccountsLockedGoldBalance_WhenAccountIsSlashedForAllOfItsLockedGold()
    public
  {
    uint256 penalty = value;
    uint256 reward = value / 2;

    whenVoteSigner_LockedGoldDelegateGovernanceVotes();

    vm.prank(delegator);
    lockedGold.delegateGovernanceVotes(delegatee1, FixidityLib.newFixedFraction(30, 100).unwrap());
    assertEq(lockedGold.getAccountNonvotingLockedGold(delegator), 1000);
    assertEq(lockedGold.getAccountTotalLockedGold(delegator), 1000);
    assertEq(lockedGold.getAccountTotalGovernanceVotingPower(delegatee1), 300);

    helper_WhenAccountIsSlashedForAllOfItsLockedGold(penalty, reward);
    assertEq(lockedGold.getAccountNonvotingLockedGold(delegator), 0);
    assertEq(lockedGold.getAccountTotalLockedGold(delegator), 0);
    assertEq(lockedGold.getAccountTotalGovernanceVotingPower(delegatee1), 300);

  }
}

Run the tests with the following command:

forge test --match-test test_POC_ShouldReduceAccountsLockedGoldBalance_WhenAccountIsSlashedForAllOfItsLockedGold -vv