delegate() delegateBalance may be lost

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L648

Vulnerability details

Impact

delegate() doesn't handle the to=address(0) case very well, and could be used to maliciously prevent others user to reset the delegate

Proof of Concept

We can set our own delegates[_vault][_from] to _to by using the delegate() method

  function _delegate(address _vault, address _from, address _to) internal {
    address _currentDelegate = _delegateOf(_vault, _from);
    if (_to == _currentDelegate) {
      revert SameDelegateAlreadySet(_to);
    }

@>  delegates[_vault][_from] = _to;

    _transferDelegateBalance(
      _vault,
      _currentDelegate,
      _to,
      uint96(userObservations[_vault][_from].details.balance)
    );

    emit Delegated(_vault, _from, _to);
  }

This method will use _transferDelegateBalance() ot transfer the current delegateBalance to _toDelegate

  function _transferDelegateBalance(
    address _vault,
    address _fromDelegate,
    address _toDelegate,
    uint96 _amount
  ) internal {
    // If we are transferring tokens from a delegated account to an undelegated account
    if (_fromDelegate != address(0) && _fromDelegate != SPONSORSHIP_ADDRESS) {
      _decreaseBalances(_vault, _fromDelegate, 0, _amount);

      // If we are delegating to the zero address, decrease total supply
      // If we are delegating to the sponsorship address, decrease total supply
      if (_toDelegate == address(0) || _toDelegate == SPONSORSHIP_ADDRESS) {
        _decreaseTotalSupplyBalances(_vault, 0, _amount);
      }
    }

    // If we are transferring tokens from an undelegated account to a delegated account
@>  if (_toDelegate != address(0) && _toDelegate != SPONSORSHIP_ADDRESS) {
      _increaseBalances(_vault, _toDelegate, 0, _amount);

      // If we are removing delegation from the zero address, increase total supply
      // If we are removing delegation from the sponsorship address, increase total supply
      if (_fromDelegate == address(0) || _fromDelegate == SPONSORSHIP_ADDRESS) {
        _increaseTotalSupplyBalances(_vault, 0, _amount);
      }
    }
  }

We know from the above method that to will not increase delegateBalance if to==address(0).

But the current protocol should default the delegate to itself if current delegate is address(0).

  function _delegateOf(address _vault, address _user) internal view returns (address) {
    address _userDelegate;

    if (_user != address(0)) {
      _userDelegate = delegates[_vault][_user];

      // If the user has not delegated, then the user is the delegate
@>    if (_userDelegate == address(0)) {
        _userDelegate = _user;
      }
    }

    return _userDelegate;
  }

So there is a problem, if the user wants to set the delegate to himself, he may execute delegate(to=address(0)). The result: the delegate becomes himself, but the delegateBalance is lost!

Malicious users may use this to prevent other users from transferring delegates.

Example. Suppose: Alice's balance= 100 and the delegate is Bob, so Alice's delegateBalance==0 and Bob's delegateBalance==100.

1. Alice executes delegate(to=address(0)) At this point, Alice's delegates become himself, but delegateBalance is still 0 (due to the problem mentioned above)

2. Jack executes delegate(to=alice) (assuming jack's balance == 100) At this point, Alice's delegateBalance will become 100.

3. Alice executes delegate(to=bob). At this point, Alice's delegateBalance will become 0 (delegateBalance = 100-100)

4. after that Jack can't reset the delegate anymore, because now Alice's delegateBalance is 0, and reset delegate will execute Alice.delegateBalance = 0 - 100 resulting in underflow

Tools Used

Recommended Mitigation Steps

  function _delegate(address _vault, address _from, address _to) internal {
+   if (_to == address(0)) _to = _from;

    address _currentDelegate = _delegateOf(_vault, _from);
    if (_to == _currentDelegate) {
      revert SameDelegateAlreadySet(_to);
    }

    delegates[_vault][_from] = _to;

    _transferDelegateBalance(
      _vault,
      _currentDelegate,
      _to,
      uint96(userObservations[_vault][_from].details.balance)
    );

    emit Delegated(_vault, _from, _to);
  }

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #293

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory