Balance invariant between individual and total twabs can be broken

[code423n4]

Lines of code

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

Vulnerability details

Impact

An edge case in the TwabController._transferBalance can cause total balance for a vault account to decrease although it did not actually decrease. This will cause the sum of individual delegateBalances for a vault to be greater than the registered total for that vault. This again will skew the odds in favor of winning a price, causing the reserve to be drained faster over time than intended.

Proof of Concept

The TwabController._transferBalance function handles the case incorrectly where _to is equal to the SPONSORSHIP_ADDRESS. First, assume the _from address is an address that has a balance and delegates to itself (the default). Then the function will decrease the balance and delegateBalance of _from. It will also decrease the total balances of the vault account (note that _toDelegate will be SPONSORSHIP_ADDRESS due to default):

    if (_from != address(0)) {
      bool _isFromDelegate = _fromDelegate == _from;

      _decreaseBalances(_vault, _from, _amount, _isFromDelegate ? _amount : 0);

      if (!_isFromDelegate && _fromDelegate != SPONSORSHIP_ADDRESS) {
        _decreaseBalances(_vault, _fromDelegate, 0, _amount);
      }

      if (
        _to == address(0) ||
        (_toDelegate == SPONSORSHIP_ADDRESS && _fromDelegate != SPONSORSHIP_ADDRESS)
      ) {
        _decreaseTotalSupplyBalances(
          _vault,
          _to == address(0) ? _amount : 0,
          (_to == address(0) && _fromDelegate != SPONSORSHIP_ADDRESS) ||
            (_toDelegate == SPONSORSHIP_ADDRESS && _fromDelegate != SPONSORSHIP_ADDRESS)
            ? _amount
            : 0
        );
      }
    }

Then, the balance and delegateBalance of SPONSORSHIP_ADDRESS will be increased by the same amount. This is not supposed to happen in the first place, as this address is not meant to have any balances. However, the issue described under #impact is due to the fact that the total balances of the vault account will not be adjusted, because the condition _toDelegate != SPONSORSHIP_ADDRESS is not met:

    if (_to != address(0)) {
      bool _isToDelegate = _toDelegate == _to;

      _increaseBalances(_vault, _to, _amount, _isToDelegate ? _amount : 0);

      if (!_isToDelegate && _toDelegate != SPONSORSHIP_ADDRESS) {
        _increaseBalances(_vault, _toDelegate, 0, _amount);
      }

      if (
        _from == address(0) ||
        (_fromDelegate == SPONSORSHIP_ADDRESS && _toDelegate != SPONSORSHIP_ADDRESS)
      ) {
        _increaseTotalSupplyBalances(
          _vault,
          _from == address(0) ? _amount : 0,
          (_from == address(0) && _toDelegate != SPONSORSHIP_ADDRESS) ||
            (_fromDelegate == SPONSORSHIP_ADDRESS && _toDelegate != SPONSORSHIP_ADDRESS)
            ? _amount
            : 0
        );
      }
    }

The ratio of _userTwab and _vaultTwabTotalSupply plays a role when determining a winner in TierCalculationLib.isWinner and the underlying model assumes the invariant to hold true:

    uint256 constrainedRandomNumber = _userSpecificRandomNumber % (_vaultTwabTotalSupply);
    uint256 winningZone = calculateWinningZone(_userTwab, _vaultContributionFraction, _tierOdds);

If the sum of individual twabs is higher than the sum, there will be more winners than intended, causing the described drainage of the reserve.

Tools Used

Manual Review

Recommended Mitigation Steps

Disallow _to to be SPONSORSHIP_ADDRESS

Assessed type

Invalid Validation

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[asselstine]

Fixed here GenerationSoftware/pt-v5-twab-controller#6