PoolTogether - Nyx's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L387-L402

Vulnerability details

Impact

Anyone can call mintYieldFee and steal funds.

Proof of Concept

if (_yieldFeePercentage != 0) {
      _increaseYieldFeeBalance(
        (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut
      );
    }

function _increaseYieldFeeBalance(uint256 _shares) internal {
    _yieldFeeTotalSupply += _shares;
  }

After using liquidate() function, _yieldFeeTotalSupply can be increased. The yield fee can serve as a buffer in case of undercollateralization of the Vault.

function mintYieldFee(uint256 _shares, address _recipient) external { //@audit anyone can call this ?
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
  }

But when undercollateralization is over, anyone can call mintYieldFee() function and steal the funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Access Control

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L296-L306 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L332-L342

Vulnerability details

Impact

Anyone can call setDrawManager() and be drawManager.

Proof of Concept

constructor(
    ConstructorParams memory params
  )
    TieredLiquidityDistributor(
      params.numberOfTiers,
      params.tierShares,
      params.canaryShares,
      params.reserveShares
    )
  {
    if (unwrap(params.smoothing) >= unwrap(UNIT)) {
      revert SmoothingGTEOne(unwrap(params.smoothing));
    }
    prizeToken = params.prizeToken;
    twabController = params.twabController;
    smoothing = params.smoothing;
    claimExpansionThreshold = params.claimExpansionThreshold;
    drawPeriodSeconds = params.drawPeriodSeconds;
    _lastClosedDrawStartedAt = params.firstDrawStartsAt;

    drawManager = params.drawManager; //@audit here
    if (params.drawManager != address(0)) {
      emit DrawManagerSet(params.drawManager);
    }
  }

If drawManager is not set in the constructor, anyone can call setDrawManager() function and can be drawManager.

function setDrawManager(address _drawManager) external { 
    if (drawManager != address(0)) {
      revert DrawManagerAlreadySet();
    }
    drawManager = _drawManager;

    emit DrawManagerSet(_drawManager);
  }

After became drawManager, attacker can steal funds with withdrawReserve() function.

function withdrawReserve(address _to, uint104 _amount) external onlyDrawManager {
    if (_amount > _reserve) {
      revert InsufficientReserve(_amount, _reserve);
    }
    _reserve -= _amount;
    _transfer(_to, _amount);
    emit WithdrawReserve(_to, _amount);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Access Control

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L352-L391

Vulnerability details

Impact

Users can alter the record of their balances.

Proof of Concept

If users delegateBalance has changed, observations will be recorded. If there is a need for new observation, new observation will be created otherwise will be overwritten.

And new observation creation will be decided in the _getNextObservationIndex() function.

if (currentPeriod == 0 || currentPeriod > newestObservationPeriod) {
      return (
        uint16(RingBufferLib.wrap(_accountDetails.nextObservationIndex, MAX_CARDINALITY)),
        newestObservation,
        true
      );
    }

newestObservationPeriod is the last observations period. currentPeriod is a period that calculated with uint32 currentTime = uint32(block.timestamp).

uint32 currentPeriod = _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, currentTime);
    uint32 newestObservationPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      newestObservation.timestamp
    );

function _getTimestampPeriod(
    uint32 PERIOD_LENGTH,
    uint32 PERIOD_OFFSET,
    uint32 _timestamp
  ) private pure returns (uint32 period) {
    if (_timestamp <= PERIOD_OFFSET) {
      return 0;
    }
    // Shrink by 1 to ensure periods end on a multiple of PERIOD_LENGTH.
    // Increase by 1 to start periods at # 1.
    return ((_timestamp - PERIOD_OFFSET - 1) / PERIOD_LENGTH) + 1;
  }

The problem is, If someone deposits a small amount frequently, currentPeriod and newestObservationPeriod will always be the same and new observation won't be created. Attackers can keep doing this until closeDraw and manipulate their balances.

According to the docs : If a draw were to start and end within a period a user would be able to alter the record of their balance for that draw by overwriting an Observation.

It is important to note that due to Observation overwriting, average balances for a period are not finalized until a period ends. Therefore if a draw ends but a period has not, a user would be able to manipulate their average balance for that final period of time after the draw ends. This would result in an inaccurate record of their balance held during the draw.

Tools Used

Manual Review

Recommended Mitigation Steps

Assessed type

Other