TwabController.transfer() could easily lead to a costly damage

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L461-L471 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L515-L582 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L473-L483

Vulnerability details

Impact

At present implementation of transfer() users could easily burn their tokens

Proof of Concept

First would be good to note that the TwabController.sol contract has the burn function that's to be used in order to burn the balance and delegateBalance for a given user, codeblock can be found here

  function burn(address _from, uint96 _amount) external {
    _transferBalance(msg.sender, _from, address(0), _amount);
  }

Also do note that what this does essentially is just call the internal _transferBalance()function which transfers tokens to the 0x0 address, as can be seen below

  function _transferBalance(address _vault, address _from, address _to, uint96 _amount) internal {
    if (_from == _to) {
      return;
    }

...ommited for brevity

      // Burn balance if we're transferring to address(0)
      // Burn delegateBalance if we're transferring to address(0) and burning from an address that is not delegating to the sponsorship address
      // Burn delegateBalance if we're transferring to an address delegating to the sponsorship address from an address that isn't delegating to the sponsorship address
      if (
        _to == address(0) ||
        (_toDelegate == SPONSORSHIP_ADDRESS && _fromDelegate != SPONSORSHIP_ADDRESS)
      ) {
        // If the user is delegating to the sponsorship address, don't adjust the total supply delegateBalance
        _decreaseTotalSupplyBalances(
          _vault,
          _to == address(0) ? _amount : 0,
          (_to == address(0) && _fromDelegate != SPONSORSHIP_ADDRESS) ||
            (_toDelegate == SPONSORSHIP_ADDRESS && _fromDelegate != SPONSORSHIP_ADDRESS)
            ? _amount
            : 0
        );
      }

...ommited for brevity
  }

Now take a look at the function used to transfer balance and delegateBalance from a given user, at L473-L483

  function transfer(address _from, address _to, uint96 _amount) external {
    _transferBalance(msg.sender, _from, _to, _amount);
  }

As seen, since there are no checks to ensure that _to != 0x0 in the case where this gets passed intentionally/unintentionally the tokens get irreversibly burnt, would only be fair to note that there is not even a resonalbe stand for someone to intentionally do this since the burn() function exists, so most probably when this occurs it's due to a very costly user error that could easily be protected by reverting in this case.

Tool used

Manual Audit

Recommended Mitigation Steps

Simple fix, add a requirement in the external transfer() function that the receiving address must not be 0x0

Assessed type

Context

[Picodes]

This would be a user mistake so downgrading to QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[asselstine]

Vaults must be able to burn twab balances on withdrawal. This is actually a non-issue