PoolTogether - KupiaSec's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1168-L1187

Vulnerability details

Impact

The exchange rate of the vaults will be decreasing and this will block core functionalities.

Proof of Concept

The exchange rate of the Vault is calculated as follows:

min(_withdrawableAssets, _totalSupplyToAssets) * _assetUnit / _totalSupplyAmount
= min(_withdrawableAssets * _assetUnit / _totalSupplyAmount, _lastRecordedExchangeRate)

So the resulting exchange rate from _currentExchangeRate method is less than or equal to _lastRecordedExchangeRate in normal situation when the total supply of current vault and the withdrawable assets from the yield vault is not zero.

_lastRecordedExchangeRate is only updated by the result of _currentExchangeRate method, so the exchange rate of the vault will be decreasing. It starts with 1, so the exchange rate will be less than 1. In this situation, maxDeposit and maxMint will return 0, so depositing to the vault will not work. The liquidation pair also can't call liquidate. Since these functionalities take important roles in the protocol, so the impact will be huge.

Tools Used

Manual Review

Recommended Mitigation Steps

I think the implementation of _currentExchangeRate is not correct at the moment, but I am not sure how to mitigate this method.

Assessed type

Error

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L566-L568

Vulnerability details

Impact

The output amount validation is not correct in Vault.liquidate(), so the method might accept invalid output amount and refuse valid output amount.

Proof of Concept

In Vault.liquidate(), there is a validation about the output share amount should be less than or equal to the liquidatable yield.

    uint256 _liquidableYield = _liquidatableBalanceOf(_tokenOut);
    if (_amountOut > _liquidableYield) 
      revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield); 

The liquidatable yield amount is in underlying asset token. So the comparison between the share amount and the underlying asset amount is not appropriate.

We could get share tokens from asset tokens via exchange rate. The vault gets _liquidableYield and mints _amountOut, so the correct asset amount equivalent to _amountOut of the share token will be _amountOut * exchange rate. The correct validation should use the asset amount and the current implementation is not correct when the exchange rate is not 1.

Tools Used

Manual Review

Recommended Mitigation Steps

We should use the underlying equivalent with the exchange rate for the validation.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Anyone can mint the yield fee using mintYieldFee().

Proof of Concept

mintYieldFee() mints the shares for yield fee.

  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 function mints the shares to the _recipient and reduces _yieldFeeTotalSupply accordingly.

But there is no access control here and anyone can steal the yield fee using this function.

Tools Used

Manual Review

Recommended Mitigation Steps

mintYieldFee() should have a whitelist for callers.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Undelegated users will lose their balances if they set delegate to themselves.

Proof of Concept

A User can set a delegate and the delegated balance of the user will be accounted on the delegate's delegated balance in the TWAB controller. The internal method _delegate will handle this functionality as follows:

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

If a user sets his delegate to himself when he is not delegated through the external delegate method, _to is address(0), and _currentDelegate will be same as _from and this passes the if statement in the next line. But this is the case when the delegates are the same, so we need to revert here.

After that, delegates storage variable is not changed. In this situation, _transferDelegateBalance will be called and this will cause the user loses his balance.

Tools Used

Manual Review

Recommended Mitigation Steps

We should revert when _currentDelegate is same as _from and _to is address(0).

Assessed type

Error

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L466-L469 https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L971-L974 https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L882-L887

Vulnerability details

Impact

Vault.mintWithPermit() uses a signature to approve the underlying asset. But the asset amount can be changed easily, so this method can be reverted and might be DoSed.

Proof of Concept

Vault.mintWithPermit() gets the share amount as an input and calculates the asset amount from the share. And then approves the asset amount with permit method.

    uint256 _assets = _beforeMint(_shares, _receiver);

    _permit(IERC20Permit(asset()), msg.sender, address(this), _assets, _deadline, _v, _r, _s);
    _deposit(msg.sender, _receiver, _assets, _shares);

The signature is generated using the exact value of the expected asset amount calculated from the share amount, and the resulting asset amount depends on the exchange rate of current vault.

  function _beforeMint(uint256 _shares, address _receiver) internal view returns (uint256) {
    if (_shares > maxMint(_receiver)) revert MintMoreThanMax(_receiver, _shares, maxMint(_receiver));
    return _convertToAssets(_shares, Math.Rounding.Up);
  }

  function _convertToAssets(
    uint256 _shares,
    Math.Rounding _rounding
  ) internal view virtual override returns (uint256) {
    return _convertToAssets(_shares, _currentExchangeRate(), _rounding);
  }

So the resulting asset amount can be different from the value of transaction start time. And even an adversary can front-run and manipulate the exchange rate. If the resulting asset amount is different from the original one, the signature will not work as expected and mintWithPermit() will revert in most cases.

Tools Used

Manual Review

Recommended Mitigation Steps

We can input upper bound of the asset amount instead of the exact value of the asset amount.

Assessed type

DoS