PoolTogether - bin2chen's results

Lines of code

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

Vulnerability details

Impact

mintYieldFee() Lack of permission control, Anyone can steal _yieldFeeTotalSupply

Proof of Concept

mintYieldFee() method can mint _yieldFeeTotalSupply to parameter _recipient

But there's no permission control.

  function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

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

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

This way anyone can steal _yieldFeeTotalSupply

Need to restrict only yieldFeeRecipient can execute this method

Tools Used

Recommended Mitigation Steps

  function mintYieldFee(uint256 _shares, address _recipient) external {
+   require(msg.sender == _yieldFeeRecipient , "not _yieldFeeRecipient");
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

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

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

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

delegate() doesn't handle the to=address(0) case very well, and could be used to maliciously prevent others user to reset the delegate

Proof of Concept

We can set our own delegates[_vault][_from] to _to by using the delegate() method

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

This method will use _transferDelegateBalance() ot transfer the current delegateBalance to _toDelegate

  function _transferDelegateBalance(
    address _vault,
    address _fromDelegate,
    address _toDelegate,
    uint96 _amount
  ) internal {
    // If we are transferring tokens from a delegated account to an undelegated account
    if (_fromDelegate != address(0) && _fromDelegate != SPONSORSHIP_ADDRESS) {
      _decreaseBalances(_vault, _fromDelegate, 0, _amount);

      // If we are delegating to the zero address, decrease total supply
      // If we are delegating to the sponsorship address, decrease total supply
      if (_toDelegate == address(0) || _toDelegate == SPONSORSHIP_ADDRESS) {
        _decreaseTotalSupplyBalances(_vault, 0, _amount);
      }
    }

    // If we are transferring tokens from an undelegated account to a delegated account
@>  if (_toDelegate != address(0) && _toDelegate != SPONSORSHIP_ADDRESS) {
      _increaseBalances(_vault, _toDelegate, 0, _amount);

      // If we are removing delegation from the zero address, increase total supply
      // If we are removing delegation from the sponsorship address, increase total supply
      if (_fromDelegate == address(0) || _fromDelegate == SPONSORSHIP_ADDRESS) {
        _increaseTotalSupplyBalances(_vault, 0, _amount);
      }
    }
  }

We know from the above method that to will not increase delegateBalance if to==address(0).

But the current protocol should default the delegate to itself if current delegate is address(0).

  function _delegateOf(address _vault, address _user) internal view returns (address) {
    address _userDelegate;

    if (_user != address(0)) {
      _userDelegate = delegates[_vault][_user];

      // If the user has not delegated, then the user is the delegate
@>    if (_userDelegate == address(0)) {
        _userDelegate = _user;
      }
    }

    return _userDelegate;
  }

So there is a problem, if the user wants to set the delegate to himself, he may execute delegate(to=address(0)). The result: the delegate becomes himself, but the delegateBalance is lost!

Malicious users may use this to prevent other users from transferring delegates.

Example. Suppose: Alice's balance= 100 and the delegate is Bob, so Alice's delegateBalance==0 and Bob's delegateBalance==100.

1. Alice executes delegate(to=address(0)) At this point, Alice's delegates become himself, but delegateBalance is still 0 (due to the problem mentioned above)

2. Jack executes delegate(to=alice) (assuming jack's balance == 100) At this point, Alice's delegateBalance will become 100.

3. Alice executes delegate(to=bob). At this point, Alice's delegateBalance will become 0 (delegateBalance = 100-100)

4. after that Jack can't reset the delegate anymore, because now Alice's delegateBalance is 0, and reset delegate will execute Alice.delegateBalance = 0 - 100 resulting in underflow

Tools Used

Recommended Mitigation Steps

  function _delegate(address _vault, address _from, address _to) internal {
+   if (_to == address(0)) _to = _from;

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

Assessed type

Context

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L781

Vulnerability details

Impact

_computeNextNumberOfTiers() Logic problem that may cause _claimExpansionThreshold to invalid.

Proof of Concept

To add tiers two conditions need to be met:

The number of tiers is increased when:

     the number of claimed standard prizes exceeds the claim threshold % of the expected number of standard prizes.
     the number of claimed canary prizes exceeds the expected number of canary prizes

The current implementation code is as follows:

  function _computeNextNumberOfTiers(uint8 _numTiers) internal view returns (uint8) {
    UD2x18 _claimExpansionThreshold = claimExpansionThreshold;

@>  uint8 _nextNumberOfTiers = largestTierClaimed + 2; // canary tier, then length
    _nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
      ? _nextNumberOfTiers
      : MINIMUM_NUMBER_OF_TIERS;

    if (_nextNumberOfTiers >= MAXIMUM_NUMBER_OF_TIERS) {
      return MAXIMUM_NUMBER_OF_TIERS;
    }

    // check to see if we need to expand the number of tiers
@>  if (
      _nextNumberOfTiers >= _numTiers &&
      canaryClaimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
      ) &&
      claimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
      )
    ) {
      // increase the number of tiers to include a new tier
      _nextNumberOfTiers = _numTiers + 1;
    }
@>  return _nextNumberOfTiers;
  }

According to the above implementation Assumption: current numberOfTiers = 3 if a claim canary tier has been executed, claimPrize(_tier = 2) After that become: canaryClaimCount = 1 largestTierClaimed = 2 _nextNumberOfTiers = largestTierClaimed + 2 = 4

According to the above implementation, even if the condition in if (_claimExpansionThreshold) is not satisfied, it will still return _nextNumberOfTiers, i.e.: return 4;.

So if someone maliciously executes claimPrize(_tier = canary tier) one time every draw, regardless of whether _claimExpansionThreshold is met, then the number of tiers will be increased by 1 every draw. The _claimExpansionThreshold limit is invalidated.

Tools Used

Recommended Mitigation Steps

  function _computeNextNumberOfTiers(uint8 _numTiers) internal view returns (uint8) {
    UD2x18 _claimExpansionThreshold = claimExpansionThreshold;

    uint8 _nextNumberOfTiers = largestTierClaimed + 2; // canary tier, then length
    _nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
      ? _nextNumberOfTiers
      : MINIMUM_NUMBER_OF_TIERS;

    if (_nextNumberOfTiers >= MAXIMUM_NUMBER_OF_TIERS) {
      return MAXIMUM_NUMBER_OF_TIERS;
    }

    // check to see if we need to expand the number of tiers
    if (
      _nextNumberOfTiers >= _numTiers &&
      canaryClaimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
      ) &&
      claimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
      )
    ) {
      // increase the number of tiers to include a new tier
      _nextNumberOfTiers = _numTiers + 1;
-   }
+   }else if(_nextNumberOfTiers > _numTiers) {
+      _nextNumberOfTiers = _numTiers
+   }

    return _nextNumberOfTiers;
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Since _yieldVault mostly calculates shares use round down when depositing, there is often a 1 wei loss of precision, which can cause the vault to go into undercollateralized mode by mistake.

Proof of Concept

When a user deposits an asset, we update _lastRecordedExchangeRate, the calculation is done by this method _currentExchangeRate()

The code is as follows:

  function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

@>  uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

    return _assetUnit;
  }

This method takes _yieldVault.maxWithdraw(address(this)) as the maximum value to calculate the current exchange rate.

If the exchange rate is lower than _assetUnit, then it goes into undercollateralized mode, where it can only be withdraw, not deposit.

So if _yieldVault is losing money, it goes into undercollateralized.

But there is one missing consideration here: As long as _yieldVault is not exclusive, there will be precision loss issues, after _yieldVault.deposit(), maxWithdraw() will lose precision, because most vaults will do rounds down shares calculations For example: depositing 1000000000, but it can only withdraw 999999999.

This leads to the problem that when the first deposit is made, it is likely to go into undercollateralized mode immediately due to the 1 wei loss.

The following code demonstrates that when a non-exclusive _yieldVault, alice is first deposited normally, it immediately enters undercollateralized mode.

add to Deposit.t.sol

  function testLossPrecision() external {
    vm.startPrank(alice);
    
    //0.Constructing a yieldVault that is already profitable
    uint256 _amount = 333e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(yieldVault), type(uint256).max);   
    yieldVault.deposit(_amount,alice);
    //profitable 0.1e18
    underlyingAsset.mint(address(yieldVault), 0.1e18);

    //1.alice deposit
    _amount = 100e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);
    console2.log("deposit:",_amount);    
    vault.deposit(_amount, alice);
    console2.log("maxWithdraw:",yieldVault.maxWithdraw(address(vault)));
    console2.log("loss :",_amount - yieldVault.maxWithdraw(address(vault)));
    console2.log("isVaultCollateralized:",vault.isVaultCollateralized());  
    return;
   }

$ forge test --match-test testLossPrecision -vvv

[PASS] testLossPrecision() (gas: 402881)
Logs:
  deposit: 100000000000000000000
  maxWithdraw: 99999999999999999999
  loss : 1
  isVaultCollateralized: false

This small loss of precision should not be treated as a loss, and we can avoid it by adding 1wei when calculation exchange rate.

Tools Used

Recommended Mitigation Steps

_yieldVault.maxWithdraw() + 1 Avoid loss of precision into undercollateralized

  function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

-   uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));
+   uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this)) + 1;

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

    return _assetUnit;
  }

Assessed type

Decimal

L-01: vault._mint() forcing conversion to uint96 may result in missing values

The current protocol Vault.sol has uint256 for shares, but uint96 is used in TwabController.sol. So in mint/burn/transfer, it is forced to convert to uint96 This causes the value to exceed type(uint96).maxifasset.decimals()>18, resulting in a loss of value. Suggest to add a method like SaftToUint96()to revert if it is larger thanuint96`.

  function _burn(address _owner, uint256 _shares) internal virtual override {
-   _twabController.burn(_owner, uint96(_shares));
+   _twabController.burn(_owner, _shares.SafeToUint96());
    _updateExchangeRate();

    emit Transfer(_owner, address(0), _shares);
  }

note:mint()/transfer() same problem

L-02: targetOf() incorrectly uses _token ! = _liquidationPair.tokenIn() Currently targetOf() uses _liquidationPair.tokenIn() but the actual comparison in liquidate() is _tokenIn ! = address(_prizePool.prizeToken()) So it's _prizePool.prizeToken() that should be used.

  function targetOf(address _token) external view returns (address) {
-   if (_token != _liquidationPair.tokenIn()) revert TargetTokenNotSupported(_token);
+   if (_tokenIn != address(_prizePool.prizeToken())) revert TargetTokenNotSupported(_token);
    return address(_prizePool);
  }

L-03: setLiquidationPair()may not be able to set the oldliquidationPair`

Use the safeApprove() method in setLiquidationPair() to set the allowance The safeApprove() method requires the current allowance to be 0 for the setting to succeed. If the liquidationPair_ has been used before, and this is the second time it is used, and there are residual allowances, it may not be possible to set it. suggest: clear the allowance to 0 before setting.

  function setLiquidationPair(
    LiquidationPair liquidationPair_
  ) external onlyOwner returns (address) {
    if (address(liquidationPair_) == address(0)) revert LPZeroAddress();

    IERC20 _asset = IERC20(asset());
    address _previousLiquidationPair = address(_liquidationPair);

    if (_previousLiquidationPair != address(0)) {
      _asset.safeApprove(_previousLiquidationPair, 0);
    }

+   _asset.safeApprove(address(liquidationPair_), 0);
    _asset.safeApprove(address(liquidationPair_), type(uint256).max);

    _liquidationPair = liquidationPair_;

    emit LiquidationPairSet(liquidationPair_);
    return address(liquidationPair_);
  }

L-04: in _withdraw() after _burn() then execute _yieldVault.withdraw() ,it may lead _updateExchangeRate() inaccuracy

In withdraw() execute _burn() first, in _yieldVault.withdraw(), code as follows.

  function _withdraw(
    address _caller,
    address _receiver,
    address _owner,
    uint256 _assets,
    uint256 _shares
  ) internal virtual override {
....

@>  _burn(_owner, _shares);

@>  _yieldVault.withdraw(_assets, address(this), address(this));
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

    emit Withdraw(_caller, _receiver, _owner, _assets, _shares);
  }

in _burn()method will execute _updateExchangeRate()

  function _burn(address _owner, uint256 _shares) internal virtual override {
@>  _twabController.burn(_owner, uint96(_shares));
@>  _updateExchangeRate();

    emit Transfer(_owner, address(0), _shares);
  }

then in _currentExchangeRate() will read _yieldVault.maxWithdraw():

  function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

@>  uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

    return _assetUnit;
  }  

This results in a wrong order :

1. vault.burn() reduces shares
2. read shares and total assets to calculate the exchange rate
3. _yieldVault.withdraw() reduces total assets

The result is that the second step of reading total assets is not correct, and the third step should be put before the second step.

Suggestion.

  function _withdraw(
    address _caller,
    address _receiver,
    address _owner,
    uint256 _assets,
    uint256 _shares
  ) internal virtual override {
....

+   _yieldVault.withdraw(_assets, address(this), address(this));

    _burn(_owner, _shares);

-   _yieldVault.withdraw(_assets, address(this), address(this));
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

    emit Withdraw(_caller, _receiver, _owner, _assets, _shares);
  }

L-05: liquidate() at risk of sandwich attack

Currently liquidate() requires _amountOut >= _vaultAssets to execute _yieldVault.deposit() if there is a residual balance in the contract. This presents some risk for a sandwich attack

Example.

1. A malicious user monitors memory pool, monitors _liquidationPair transfer assets to the contract, and executes liquidate().
2. front-run the transaction in step 1, transferring an asset larger than _amountOut.
3. wait until the first transaction, because _amountOut >= _vaultAssets, will not execute _yieldVault.deposit(), the funds are still in the contract
4. Malicious users are using the balance left in the contract by executing vault.deposit().

Suggest to remove _amountOut >= _vaultAssets restriction.

  function liquidate(
    address _account,
    address _tokenIn,
    uint256 _amountIn,
    address _tokenOut,
    uint256 _amountOut
  ) public virtual override returns (bool) {
...

    uint256 _vaultAssets = IERC20(asset()).balanceOf(address(this));

-   if (_vaultAssets != 0 && _amountOut >= _vaultAssets) {
+   if (_vaultAssets != 0) {
      _yieldVault.deposit(_vaultAssets, address(this));
    }

    _mint(_account, _amountOut);

    return true;
  }