code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Undelegated users will lose their balances if they set delegate to themselves.

Proof of Concept

A User can set a delegate and the delegated balance of the user will be accounted on the delegate's delegated balance in the TWAB controller. The internal method _delegate will handle this functionality as follows:

  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);
  }

If a user sets his delegate to himself when he is not delegated through the external delegate method, _to is address(0), and _currentDelegate will be same as _from and this passes the if statement in the next line. But this is the case when the delegates are the same, so we need to revert here.

After that, delegates storage variable is not changed. In this situation, _transferDelegateBalance will be called and this will cause the user loses his balance.

Tools Used

Manual Review

Recommended Mitigation Steps

We should revert when _currentDelegate is same as _from and _to is address(0).

Assessed type

Error

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #293

c4-judge commented 11 months ago

Picodes marked the issue as satisfactory