PoolTogether - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Vault.mintYieldFee function can be called by anyone, so fees can be stolen

Proof of Concept

Owner fees are accrued inside _yieldFeeTotalSupply variable. So he can mint shares in the vault after by calling mintYieldFee function.

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

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

As you can see this function can be called by anyone and any _recipient can be provided, which allows anyone to steal owner's fees.

Tools Used

VsCode

Recommended Mitigation Steps

Allow to call this function for _yieldFeeRecipient only.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Delegating to address 0 is permanent action, user will not be able to undo it and will not be able to withdraw anymore

Proof of Concept

_delegate function allows user to delegate his balance to another address. Any address is supported, so user can provide 0 address. https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L648-L664

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

Then delegates[_vault][_from] will be set to 0. And balance of user will be decreased, so it will be 0. This is the problem for 2 reasons. First, is that in case if delegator is address 0, then total supply is decreased, which means that this balance is donated, similar to SPONSORSHIP_ADDRESS. But for this purpose protocol uses exactly SPONSORSHIP_ADDRESS and there is no need to use another one.

Second problem is that once user set delegate as address 0, then when he would like to delegate it to someone else, then incorrect accounting will occur, because _delegateOf function will always return user address in case if his delegator is address 0. That means that user address will be set to the _transferDelegateBalance function as _fromDelegate param and function will try to decrease delegated balance of user, which actually doesn't have it. Function should decrease balance of address 0. So the function will revert. Also as 0 will never be passed to the _transferDelegateBalance function, total supply will also not be increased.

This problem will also affect withdrawing and user will not be able to withdraw anymore for same reasons.

I think it's medium severity, because it's unlikely to occur. But in case if it will, then user will lose all funds.

Tools Used

VsCode

Recommended Mitigation Steps

I recommend to not allow to delegate to address 0.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Vault is not compatible with some erc4626 vaults. Depositors can loose funds.

Proof of Concept

Anyone can build Vault with underlying vault inside, which should earn yields. When user deposits/withdraws then _convertToShares function is used to determine amount of shares that user will receive for provided assets amount. This function calls _currentExchangeRate to find out current rate.

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

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

As you can see in order to find current exchange rate _yieldVault.maxWithdraw(address(this)) is used. In case if this value(which is supposed to be full amount of deposits + yields inside _yieldVault) is less than _totalSupplyAmount(which is total supply * _lastRecordedExchangeRate), then rate will be decreased, which means that vault lost funds and users should receive less when withdraw. Later, this new _currentExchangeRate will be stored as _lastRecordedExchangeRate.

Now when i explained how rate is changed i can explain the problem with some erc4626 vaults.

There are some erc4626 vaults as DnGmxSeniorVault, that collect deposited funds and borrow them. When you call maxWithdraw for such vaults, then in case if not enough funds are present, because of some borrow percentage on vault, then amount returned can be less than real balance of caller. In case if such wrong value is returned, then depositors of Pool vault will face losses as their exchange rate will be less than 1. When DnGmxSeniorVault will again have enough balance(when debt is repaid by jn vault), then exchange rate will be 1 again.

Another erc4626 vaults that can create problems are vaults that have withdraw limit. In that case if Pool vault has balance inside yield vault that is bigger than borrow limit, then depositors will face same problem which leads to loose of funds.

Tools Used

VsCode

Recommended Mitigation Steps

You need to consider cases, when some vaults can't be used as yield vaults and aware vault creators about that.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Winner can make claim bot spend whole gas amount and doesn't receive any fees

Proof of Concept

Claim bots are going to batch a lot of prize claims in 1 transaction in order to receive fees and spend less gas for execution.

This call is then passed to Vault._claimPrize function. https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1043-L1078

  function _claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    uint96 _fee,
    address _feeRecipient
  ) internal returns (uint256) {
    VaultHooks memory hooks = _hooks[_winner];
    address recipient;
    if (hooks.useBeforeClaimPrize) {
      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
    } else {
      recipient = _winner;
    }


    uint prizeTotal = _prizePool.claimPrize(
      _winner,
      _tier,
      _prizeIndex,
      recipient,
      _fee,
      _feeRecipient
    );


    if (hooks.useAfterClaimPrize) {
      hooks.implementation.afterClaimPrize(
        _winner,
        _tier,
        _prizeIndex,
        prizeTotal - _fee,
        recipient
      );
    }


    return prizeTotal;
  }

This function uses hooks that are set by winner. As you can see in case if hook exists, then the call will be done to it. Malicious winner can set such hook, that will run all provided gas to revert with out of gas error. Then whole claim transaction will revert as well and as result, bot will spend all gas, but will not receive any rewards.

Tools Used

VsCode

Recommended Mitigation Steps

Maybe hook should have some gas limit to avoid this problem.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Vault.liquidate doesn't check that provide pool amount to the prize pool is enough to mint asked amount of tokens by liquidator

Proof of Concept

When vault has generated enough yields, then liquidator can call liquidate function. https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L550-L587

  function liquidate(
    address _account,
    address _tokenIn,
    uint256 _amountIn,
    address _tokenOut,
    uint256 _amountOut
  ) public virtual override returns (bool) {
    _requireVaultCollateralized();
    if (msg.sender != address(_liquidationPair))
      revert LiquidationCallerNotLP(msg.sender, address(_liquidationPair));
    if (_tokenIn != address(_prizePool.prizeToken()))
      revert LiquidationTokenInNotPrizeToken(_tokenIn, address(_prizePool.prizeToken()));
    if (_tokenOut != address(this))
      revert LiquidationTokenOutNotVaultShare(_tokenOut, address(this));
    if (_amountOut == 0) revert LiquidationAmountOutZero();


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


    _prizePool.contributePrizeTokens(address(this), _amountIn);


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


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


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


    _mint(_account, _amountOut);


    return true;
  }

This actually means, that liquidator has transferred _amountIn amount of POOL token to prize pool, so _prizePool.contributePrizeTokens function can be called by vault and that vault should mint _amountOut of shares to the liquidator.

As you can see the function has some validation, but it doesn't check that _amountIn/_amountOut is fair ratio. As liquidator can be provided and changed by owner of vault, that means that in such case he has ability to withdraw all yields of user. He just can set liquidator that provided 1 wei of POOl token to prize pool and mints all yields.

Tools Used

VsCode

Recommended Mitigation Steps

There should be some price check.

Assessed type

Error

Lines of code

https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L60-L83

Vulnerability details

Impact

Claimer.claimPrizes can be frontrunned in order to make losses for the claim bot

Proof of Concept

All prizes in the system should be claimed during draw period. Anyone can call claimPrizes function for any winner and prize. But the idea of protocol is to incentivize claim bots to do all claims instead of users. These should benefit from batching claims together in 1 call.

Also Claimer contract is using vrgda which can increase/decrease fee for the claimer, depending of amount of already claimed prizes. That actually means that in case if not enough prizes are claimed in some point of time, then fee will be increased to incentivize more bots.

So as bots will be trying to batch a lot of prize claims together, that means that they will spent a lot on gas. Malicious actor can see which prizes bot is going to claim and frontrun him with last prize is the list. As result claiming will fail and bot will face losses.

Another reason, why malicious actor can do that is to make vrgda to provide bigger fees. So he can create a bot that will block claims of another bots with big amount of claims in the batch, in order to wait and get better prices and claim those prize by himself and receive fees.

Tools Used

VsCode

Recommended Mitigation Steps

I guess that in case if price is already claimed, then you can return early from the PrizePool.claimPrize function and emit some event. In this case bot will not receive fee for already claimed prize and his tx will continue with other prizes in the batch.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

depositWithPermit and mintWithPermit are allowed to be called by permit creator only. No any other contracts will be able to execute these function on behalf of signer.

Proof of Concept

depositWithPermit function allows to provide signed permit in order to receive approve and deposit funds into the vault.

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

  function depositWithPermit(
    uint256 _assets,
    address _receiver,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external returns (uint256) {
    _permit(IERC20Permit(asset()), msg.sender, address(this), _assets, _deadline, _v, _r, _s);
    return deposit(_assets, _receiver);
  }

This function calls _permit and pass msg.sender as _owner to that function. https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1093-L1104

  function _permit(
    IERC20Permit _asset,
    address _owner,
    address _spender,
    uint256 _assets,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) internal {
    _asset.permit(_owner, _spender, _assets, _deadline, _v, _r, _s);
  }

This means that signer can be only the same person that called depositWithPermit function.

However the purpose of permit is to allow someone to sign approve signature, so that this signature can be used by another contract to call some function on behalf of signer.

In this case, anyone should be able to sign permit for the vault, and vault should check that _receiver is same who signed permit.

Tools Used

VsCode

Recommended Mitigation Steps

Use _receiver instead of msg.sender.

  function depositWithPermit(
    uint256 _assets,
    address _receiver,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external returns (uint256) {
    _permit(IERC20Permit(asset()), _receiver, address(this), _assets, _deadline, _v, _r, _s);
    return deposit(_assets, _receiver);
  }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

TwabController._transferBalance doesn't delegate to top delegator, which breaks twab acounting.

Proof of Concept

TwabController._transferBalance function is called when any funds are minted/burnt/transferred. One of the things that it should to is to manage delegating. https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L515-L582

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


    // If we are transferring tokens from a delegated account to an undelegated account
    address _fromDelegate = _delegateOf(_vault, _from);
    address _toDelegate = _delegateOf(_vault, _to);
    if (_from != address(0)) {
      bool _isFromDelegate = _fromDelegate == _from;


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


      // If the user is not delegating to themself, decrease the delegate's delegateBalance
      // If the user is delegating to the sponsorship address, don't adjust the delegateBalance
      if (!_isFromDelegate && _fromDelegate != SPONSORSHIP_ADDRESS) {
        _decreaseBalances(_vault, _fromDelegate, 0, _amount);
      }


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


    // If we are transferring tokens to an address other than address(0)
    if (_to != address(0)) {
      bool _isToDelegate = _toDelegate == _to;


      // If the user is delegating to themself, increase their delegateBalance
      _increaseBalances(_vault, _to, _amount, _isToDelegate ? _amount : 0);


      // Otherwise, increase their delegates delegateBalance if it is not the sponsorship address
      if (!_isToDelegate && _toDelegate != SPONSORSHIP_ADDRESS) {
        _increaseBalances(_vault, _toDelegate, 0, _amount);
      }


      // Mint balance if we're transferring from address(0)
      // Mint delegateBalance if we're transferring from address(0) and to an address not delegating to the sponsorship address
      // Mint delegateBalance if we're transferring from an address delegating to the sponsorship address to an address that isn't delegating to the sponsorship address
      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
        );
      }
    }
  }

As you can see function finds delegators of from and to addresses. And then increases/decreases twab(delegated) balances for them. https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L590-L603

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

This function just retrieves delegator of user, but it doesn't consider scenario, that this delegator can have another delegator. Because of that accounting will be wrong.

Example. 1.A delegates to B, B delegates to C, C delegates to D. 2.A mints 100 shares. 3.delegator of A is B and delegator of B is C, so this 100 shares are delegated to C. 4.But it's incorrect, because C delegates to D and now twab balance of D is not increased and his win odds are not increased as well.

Tools Used

VsCode

Recommended Mitigation Steps

You need to use top delegator fro both to and from accounts.

Assessed type

Error