userTotalStaked invariant will be broken due to vulnerable implementations in release()

criticalCode4rena

Lines of code

https://github.com/code-423n4/2024-03-gitcoin/blob/6529b351cd72a858541f60c52f0e5ad0fb6f1b16/id-staking-v2/contracts/IdentityStaking.sol#L573-L574

Vulnerability details

Impact

userTotalStaked invariant will be broken due to vulnerable implementations in release(). Users might lose funds due to underflow errors in withdraw methods.

Proof of Concept

According to readme, userTotalStaked invariant should always hold true. userTotalStaked[address] = selfStakes[address].amount + sum(communityStakes[address][x].amount for all x staked on by this address)

However, this can be broken in release() flow due to userTotalStaked is not updated together with selfStakes[address].amount or communityStakes[address][x].amount.

solidity📋
//id-staking-v2/contracts/IdentityStaking.sol
  function release(
    address staker,
    address stakee,
    uint88 amountToRelease,
    uint16 slashRound
  ) external onlyRole(RELEASER_ROLE) whenNotPaused {
...
    if (staker == stakee) {
...
      selfStakes[staker].slashedAmount -= amountToRelease;
      //@audit selfStakes[staker].amount is updated but `userTotalStaked` is not
|>    selfStakes[staker].amount += amountToRelease;
    } else {
...
      communityStakes[staker][stakee].slashedAmount -= amountToRelease;
      //@audit communityStakes[staker].amount is updated but `userTotalStaked` is not
|>    communityStakes[staker][stakee].amount += amountToRelease;
    }
...

(https://github.com/code-423n4/2024-03-gitcoin/blob/6529b351cd72a858541f60c52f0e5ad0fb6f1b16/id-staking-v2/contracts/IdentityStaking.sol#L562-L563)

Note: For comparison, in other flows such as staking, withdrawing and slashing, userTotalStaked is always updated in sync with selfStakes/communityStakes.

POC:

1. Alice selfStakes() 100000 ether and communityStakes() 100000 at round 1.
2. Alice's selfStake and communityStake are slashed by 80% each.
3. Alice appealed and was released the full slashed amount. Alice's staked balance is restored to 100000 ether each. But userTotalStaked is not restored.
4. Alice's unlocked but cannot withdraw the 100000x2 ether balance due to underflow. She can only withdraw 20000x2 ether.

See test below: In id-staking-v2/test/IdentityStaking.ts, first add import { PANIC_CODES} from "@nomicfoundation/hardhat-chai-matchers/panic";. Then copy this test inside describe("slashing/releasing/burning tests", function () {.

ts📋
it.only("userTotalStaked is broken, user lose funds", async function(){
  //Step2: Round1 - slash Alice's self and community stake of 80000 each
  await this.identityStaking
  .connect(this.owner)
  .slash(
    this.selfStakers.slice(0, 1),
    this.communityStakers.slice(0, 1),
    this.communityStakees.slice(0, 1),
    80,
  );
  //Step2: Round1 - Alice's community/self stake is 20000 after slashing
  expect(
    (
      await this.identityStaking.communityStakes(
        this.communityStakers[0],
        this.communityStakees[0],
      )
    ).amount,
  ).to.equal(20000);
  //Step2: Round1 - total slashed amount 80000 x 2
  expect(await this.identityStaking.totalSlashed(1)).to.equal(160000);
  //Step3: Round1 - Alice appealed and full slash amount is released 80000 x 2
  await this.identityStaking
  .connect(this.owner)
  .release(this.selfStakers[0], this.selfStakers[0], 80000, 1);

  await this.identityStaking
  .connect(this.owner)
  .release(this.communityStakers[0], this.communityStakees[0], 80000, 1);


  //Step3: Round1 - After release, Alice has full staked balance 100000 x 2 
  expect((await this.identityStaking.selfStakes(this.selfStakers[0])).amount).to.equal(100000);
  expect((await this.identityStaking.communityStakes(this.communityStakers[0],this.communityStakees[0])).amount).to.equal(100000);
  expect(await this.identityStaking.totalSlashed(1)).to.equal(0);

  // Alice's lock expired
  await time.increase(twelveWeeksInSeconds + 1);
  //Step4: Alice trying to withdraw 100000 x 2 from selfStake and communityStake. Tx reverted with underflow error. 
  await  expect((this.identityStaking.connect(this.userAccounts[0]).withdrawSelfStake(100000))).to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW);
  await  expect((this.identityStaking.connect(this.userAccounts[0]).withdrawCommunityStake(this.communityStakees[0],100000))).to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW);
  //Step4: Alice could only withdraw 20000 x 2. Alice lost 80000 x 2.
  await this.identityStaking.connect(this.userAccounts[0]).withdrawSelfStake(20000);
  await this.identityStaking.connect(this.userAccounts[0]).withdrawCommunityStake(this.communityStakees[0],20000);


 })

Tools Used

Manual Hardhat

Recommended Mitigation Steps

In release(), also update userTotalStaked.

Assessed type

Other