Juicebox contest - csanuragjain's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/main/contracts/JB721TieredGovernance.sol#L177

Vulnerability details

Impact

The _totalTierCheckpoints does not get updated if user has delegated its votes to address(0). This becomes a problem for Governance since for a tier, the total votes would become more than existing vote. In case if a significant portion of vote has been delegated to address 0, then the voting system will get impacted

Proof of Concept

1. User A delegates his voting units to address(0) using setTierDelegate function

function setTierDelegate(address _delegatee, uint256 _tierId) public virtual override {
    _delegateTier(msg.sender, _delegatee, _tierId);
  }

2. This internally calls _delegateTier function which then calls _moveTierDelegateVotes function with voting units of User A

function _delegateTier(
    address _account,
    address _delegatee,
    uint256 _tierId
  ) internal virtual {
    // Get the current delegatee
    address _oldDelegate = _tierDelegation[_account][_tierId];

    // Store the new delegatee
    _tierDelegation[_account][_tierId] = _delegatee;

    emit DelegateChanged(_account, _oldDelegate, _delegatee);

    // Move the votes.
    _moveTierDelegateVotes(
      _oldDelegate,
      _delegatee,
      _tierId,
      _getTierVotingUnits(_account, _tierId)
    );
  }

3. Observe there is no check to verify that new _delegatee is not address 0

4. Now _moveTierDelegateVotes function is called which updates the _delegateTierCheckpoints for this user on the mentioned tier

function _moveTierDelegateVotes(
    address _from,
    address _to,
    uint256 _tierId,
    uint256 _amount
  ) internal {
    // Nothing to do if moving to the same account, or no amount is being moved.
    if (_from == _to || _amount == 0) return;

    // If not moving from the zero address, update the checkpoints to subtract the amount.
    if (_from != address(0)) {
      (uint256 _oldValue, uint256 _newValue) = _delegateTierCheckpoints[_from][_tierId].push(
        _subtract,
        _amount
      );
      emit TierDelegateVotesChanged(_from, _oldValue, _newValue, _tierId, msg.sender);
    }

    // If not moving to the zero address, update the checkpoints to add the amount.
    if (_to != address(0)) {
      (uint256 _oldValue, uint256 _newValue) = _delegateTierCheckpoints[_to][_tierId].push(
        _add,
        _amount
      );
      emit TierDelegateVotesChanged(_to, _tierId, _oldValue, _newValue, msg.sender);
    }
  }

5. Since _to is address 0 (user is delegating to 0 address) so _delegateTierCheckpoints[_to][_tierId] will not be updated and only _delegateTierCheckpoints[_from][_tierId] will be updated which is correct

6. The problem here is that now this tier has in a way burned all the voting units for User A which means this tier has now lesser number of voting units. This should have been updated in _totalTierCheckpoints (which tracks total voting units in tier) but it is not done within this flow

7. This means after this transaction, total voting units in this checkpoint will be including the non existing votes (from user A delegation to 0 address)

8. So lets say in tier id1

total votes : 1000
user A voting units: 600

User A delegates to address 0 

total votes : 1000
user A voting units: 0 (since _delegateTierCheckpoints[_from][_tierId].push( _subtract, _amount );)

Governance voting starts
Only 1000-600=400 votes can be placed
If governance required atleast 50% consensus from total votes or required 50% participation of overall votes then this would never reach

Recommended Mitigation Steps

Do not allow User to delegate to 0 address if they have some voting units. If required then kindly update _totalTierCheckpoints before delegating to 0 address