PoolTogether - dacian's results

Lines of code

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

Vulnerability details

Impact

Attacker can mint themselves free Vault shares via Vault.mintYieldFee() when the vault is collateralized and _yieldFeeTotalSupply > 0.

Proof of Concept

Consider Vault.mintYieldFee():

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:

• can be called by anyone,
• allows the passing of an arbitrary address in _recipient parameter,
• mints _shares to that arbitrary address via _mint(_recipient, _shares).

Whenever _yieldFeeTotalSupply > 0, an attacker can mint themselves free shares by calling mintYieldFee(_yieldFeeTotalSupply, attacker_address).

Tools Used

Manual review

Recommended Mitigation Steps

The comment above the function indicates that the minted shares should go to _yieldFeeRecipient, so the fix is simple: _mint(_yieldFeeRecipient, _shares)

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L80 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L621 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1058 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L473

Vulnerability details

Impact

Claimers will create unexpected & undesired tax liabilities for winning users which will result in disastrous real-world financial consequences for winning users.

Proof of Concept

From the official docs:

"PoolTogether V5 incentivizes Claimers to claim prizes on behalf of winners. This means that anyone holding prize-wrapped assets will simply receive tokens; they don't need to run a transaction."

This is reflected in the code; Claimer.claimPrizes() -> Vault.claimPrizes() -> Vault.claimPrize() -> PrizePool.claimPrize() which transfers the reward tokens to the recipient who was not the transaction initiator as the transaction was initiated by the Claimer.

In many of the world's tax jurisdictions this receipt of tokens will be considered taxable income incurring a tax liability for the recipient. The taxable amount is commonly the fiat value of the received tokens at the time they were received even if unsold. Claimers will be constantly creating unexpected & potentially undesired real-world tax liabilities for other users.

This can be especially problematic if a user doesn't immediately sell the received tokens (since they could be unaware as they didn't initiate the transaction & they are using PoolTogether as a long-term savings vehicle to HODL). When the user eventually files their tax return if their tax authority has picked up these received tokens, they'll be hit with a tax liability for them with the fiat value on the day they were received.

If by that time the value of the received tokens has decreased substantially (received in bull market, file taxes in bear market), such users would be left with a tax liability that couldn't be covered by liquidating their tokens, which would incur serious financial losses for the users.

For example if the rewards were received using PoolTogether's POOL Token, the tax liability could be very substantial as POOL is currently down -97% to around $0.70 from its all-time bull-market high around $27.65, meaning that unexpected tax liabilities could incur life-destroying real-world financial losses for users who are caught unawares.

Tools Used

Manual review

Recommended Mitigation Steps

Users must be allowed to opt-out of automatically receiving winning token rewards from transactions initiated by Claimers. Winning tokens could accumulate for such users in a vault contract, and users could call a function on that contract to receive their accumulated winnings. This would allow users to control both the time and the price at which tax liability is incurred on their winnings.

Currently a user could use a VaultHook that returns another address they control from beforeClaimPrize() to send their rewards to another address they control / their own vault contract, but this would likely not be sufficient to avoid tax liability since the rewards are just going to another address they control / a contract they deployed.

It would be much cleaner & more defensible if users were able to explicitly opt-out of automatically receiving rewards from Claimers and for users who had opted out, the PrizePool.claimPrize() put these rewards into a Vault contract which PoolTogether deployed, to be redeemed by the user at a time of their choosing.

Assessed type

Other