PoolTogether - peanuts's results

Lines of code

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

Vulnerability details

Impact

Anyone can steal the yield fees.

Proof of Concept

The mintYieldFee() function in Vault.sol has no modifier and thus anyone can call this function to mint yield fees to themselves.

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

There is an assigned yieldFeeRecipient_ done in the constructor and the owner can change the yield fee recipient by calling setYieldFeeRecipient(). The fees should only go to the yield fee recipient.

  constructor(
    _setYieldFeeRecipient(yieldFeeRecipient_);

  function setYieldFeeRecipient(address yieldFeeRecipient_) external onlyOwner returns (address) {
    address _previousYieldFeeRecipient = _yieldFeeRecipient;
    _setYieldFeeRecipient(yieldFeeRecipient_);


    emit YieldFeeRecipientSet(_previousYieldFeeRecipient, yieldFeeRecipient_);
    return yieldFeeRecipient_;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure mintYieldFee() has an OnlyOwner modifier or lock the recipient to the yieldFeeRecipient_ set in _setYieldFeeRecipient()

  function _setYieldFeeRecipient(address yieldFeeRecipient_) internal {
    _yieldFeeRecipient = yieldFeeRecipient_;
  }

Assessed type

Context

Context

Focused solely on the Vault, TWAB controller and TWAB library.

Approach taken in evaluating the codebase

• Read through the documentation and watch the video walkthrough of the code
• Examine the interaction between the Vault, TWAB Library
• Focus on the Vault and see if there is any ERC4626 complications
• Try to break the Vault

Architecture Recommendations

Check the resultant amount of shares/assets when depositing / withdrawing from the vault. Make sure that the shares and assets received after converting is not zero. If it is zero, make sure to revert. For example:

  function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
    if (_assets > maxDeposit(_receiver))
      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));


    uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);
+   require(_shares != 0, "Zero Shares");
    _deposit(msg.sender, _receiver, _assets, _shares);

Fee-on-transfer tokens, rebasing tokens and low-decimal tokens may affect the vault and accounting so maybe can check the decimal places of the asset during constructions, and set a whitelist of the assets used.

Also, safeApprove is used, and I believe USDT doesn't work through safeApprove but have to set the value to zero first.  

Codebase quality analysis

Codebase is pretty complicated, so it'll be good if there is a step-by-step walkthrough for all the points and examples for the Math used.

Good amount of NATSPECs, the external calls is a little hard to grasp, especially jumping from Vault to TwAB controller to TWAB library and back. I see that alot of thought and effort is put into this new version, with the extensive test coverage as well.

Mechanism Review

The two-vault scenario is pretty novel and interesting, but extremely confusing to me. I believe that the Vault doesn't actually mint shares, but call the TWAB controller to store the amount of shares in the vault. The owner still receives shares from the yield vault and virtual shares from the TWAB controller. I would like to see how it plays out, especially since I am also a user of PoolTogether.

Centralization Risks

The Vault is permissionless and the Yield Vault is also not checked. This invites alot of complication and trust issues with the user and the protocol because users has to trust that the Vault is not malicious in order to deposit and interact with the protocol. Would be good if there are some Protocol-initiated Vaults to increase user's trust.

Systemic Risks

I don't see a failsafe measure to restore funds in case funds get stuck in the protocol. Maybe can introduce the emergency withdraw function through a multisig in case any part of the protocol gets compromised, especially so when the protocol is going permissionless

Time spent:

10 hours