code-423n4 / 2024-03-gitcoin-mitigation-findings

0 stars 0 forks source link

PR12 Unmitigated #17

Closed liveactionllama closed 7 months ago

liveactionllama commented 7 months ago

This adds a missing Release event and changes the Slash event.

Description

The owner implemented a missing address to correspondent the slashed stakee in case of a community stake slashing.

  event Slash(address indexed staker, address indexed stakee, uint88 amount, uint16 round);

Currently the slashing function can emit a slashing event which depends on the kind of executed slash.

The main problem we are facing is that it's hard to differ whether a self or community stake was slashed:

  1. As of now we only have one Slash event, which is emitted on both slashed stakes.
  2. Slashing selfStake emits the same address two times even tho it is needed only ones.
      emit Slash(staker, staker, slashedAmount, currentSlashRound);
      emit Slash(staker, stakee, slashedAmount, currentSlashRound);

Recommendation

l recommend emitting two different events depending on the situation of which kind of stake is slashed:

+ event SlashSelfStake(address indexed staker, uint88 amount, uint16 round);
+ event SlashCommunityStake(address indexed staker, address indexed stakee, uint88 amount, uint16 round);

Ultimately the change won't affect any functionality and everything will work as intended like before, on the other hand the slashing events will be more recognisable.

  function slash(
    address[] calldata selfStakers,
    address[] calldata communityStakers,
    address[] calldata communityStakees,
    uint88 percent
  ) external onlyRole(SLASHER_ROLE) whenNotPaused {
    if (percent > 100 || percent == 0) {
      revert InvalidSlashPercent();
    }

    uint256 numSelfStakers = selfStakers.length;
    uint256 numCommunityStakers = communityStakers.length;

    if (numCommunityStakers != communityStakees.length) {
      revert StakerStakeeMismatch();
    }

    for (uint256 i = 0; i < numSelfStakers; i++) {
      address staker = selfStakers[i];
      uint88 slashedAmount = (percent * selfStakes[staker].amount) / 100;

      Stake storage sStake = selfStakes[staker];

      if (sStake.slashedInRound != 0 && sStake.slashedInRound != currentSlashRound) {
        if (sStake.slashedInRound == currentSlashRound - 1) {
          // If this is a slash from the previous round (not yet burned), move
          // it to the current round
          totalSlashed[currentSlashRound - 1] -= sStake.slashedAmount;
          totalSlashed[currentSlashRound] += sStake.slashedAmount;
        } else {
          // Otherwise, this is a stale slash and can be overwritten
          sStake.slashedAmount = 0;
        }
      }

      totalSlashed[currentSlashRound] += slashedAmount;

      sStake.slashedInRound = currentSlashRound;
      sStake.slashedAmount += slashedAmount;
      sStake.amount -= slashedAmount;

      userTotalStaked[staker] -= slashedAmount;

+     emit SlashSelfStake(staker, slashedAmount, currentSlashRound);
    }

    for (uint256 i = 0; i < numCommunityStakers; i++) {
      address staker = communityStakers[i];
      address stakee = communityStakees[i];
      uint88 slashedAmount = (percent * communityStakes[staker][stakee].amount) / 100;

      Stake storage comStake = communityStakes[staker][stakee];

      if (comStake.slashedInRound != 0 && comStake.slashedInRound != currentSlashRound) {
        if (comStake.slashedInRound == currentSlashRound - 1) {
          // If this is a slash from the previous round (not yet burned), move
          // it to the current round
          totalSlashed[currentSlashRound - 1] -= comStake.slashedAmount;
          totalSlashed[currentSlashRound] += comStake.slashedAmount;
        } else {
          // Otherwise, this is a stale slash and can be overwritten
          comStake.slashedAmount = 0;
        }
      }

      totalSlashed[currentSlashRound] += slashedAmount;

      comStake.slashedInRound = currentSlashRound;
      comStake.slashedAmount += slashedAmount;
      comStake.amount -= slashedAmount;

      userTotalStaked[staker] -= slashedAmount;

+     emit SlashCommunityStake(staker, stakee, slashedAmount, currentSlashRound);
    }
  }
liveactionllama commented 7 months ago

Note: per discussion with the judge, sponsor, and participating wardens, the full scope for this competition was initially a bit unclear. As such, the participating wardens have all agreed to review items outside of the original active competition phase. Adding their work to this findings repo for further review by all parties.

GalloDaSballo commented 7 months ago

My 2 cents: if the event would current emit:

Then the refactoring is not necessary

nutrina commented 7 months ago

Yes, it is implemented as @GalloDaSballo suggested, staker and stakee are the same on selfSlash and different on communitySlash

GalloDaSballo commented 7 months ago

Yes, it is implemented as @GalloDaSballo suggested, staker and stakee are the same on selfSlash and different on communitySlash

I suggest ignoring the suggestion then!

c4-judge commented 7 months ago

GalloDaSballo marked the issue as nullified