Delegating to address 0 is permanent action, user will not be able to undo it and will not be able to withdraw anymore

[code423n4]

Lines of code

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

Vulnerability details

Impact

Delegating to address 0 is permanent action, user will not be able to undo it and will not be able to withdraw anymore

Proof of Concept

_delegate function allows user to delegate his balance to another address. Any address is supported, so user can provide 0 address. https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L648-L664

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

Then delegates[_vault][_from] will be set to 0. And balance of user will be decreased, so it will be 0. This is the problem for 2 reasons. First, is that in case if delegator is address 0, then total supply is decreased, which means that this balance is donated, similar to SPONSORSHIP_ADDRESS. But for this purpose protocol uses exactly SPONSORSHIP_ADDRESS and there is no need to use another one.

Second problem is that once user set delegate as address 0, then when he would like to delegate it to someone else, then incorrect accounting will occur, because _delegateOf function will always return user address in case if his delegator is address 0. That means that user address will be set to the _transferDelegateBalance function as _fromDelegate param and function will try to decrease delegated balance of user, which actually doesn't have it. Function should decrease balance of address 0. So the function will revert. Also as 0 will never be passed to the _transferDelegateBalance function, total supply will also not be increased.

This problem will also affect withdrawing and user will not be able to withdraw anymore for same reasons.

I think it's medium severity, because it's unlikely to occur. But in case if it will, then user will lose all funds.

Tools Used

VsCode

Recommended Mitigation Steps

I recommend to not allow to delegate to address 0.

Assessed type

Error

[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