PoolTogether - neumo's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L254-L296 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L665-L683

Vulnerability details

Impact

Vaults can be created permissionlessly by anyone. A malicious vault creator has a couple of ways to give ultimited appoval over the vault's underlying asset to a contract in his control. This way, once the vault has enough deposits, the owner can rugpull all the funds.

Proof of Concept

The places the bad actor can use for this attack are:

The constructor

254	constructor(
255		IERC20 asset_,
256		string memory name_,
257		string memory symbol_,
258		TwabController twabController_,
259		IERC4626 yieldVault_,
260		PrizePool prizePool_,
261		address claimer_,
262		address yieldFeeRecipient_,
263		uint256 yieldFeePercentage_,
264		address owner_
265	) ERC4626(asset_) ERC20(name_, symbol_) ERC20Permit(name_) Ownable(owner_) {
266		if (address(twabController_) == address(0)) revert TwabControllerZeroAddress();
267		if (address(yieldVault_) == address(0)) revert YieldVaultZeroAddress();
268		if (address(prizePool_) == address(0)) revert PrizePoolZeroAddress();
269		if (address(owner_) == address(0)) revert OwnerZeroAddress();
270		
271		_twabController = twabController_;
272		_yieldVault = yieldVault_;
273		_prizePool = prizePool_;
274		
275		_setClaimer(claimer_);
276		_setYieldFeeRecipient(yieldFeeRecipient_);
277		_setYieldFeePercentage(yieldFeePercentage_);
278		
279		_assetUnit = 10 ** super.decimals();
280		
281		    // Approve once for max amount
282		asset_.safeApprove(address(yieldVault_), type(uint256).max);
283		
284		emit NewVault(
285			asset_,
286			name_,
287			symbol_,
288			twabController_,
289			yieldVault_,
290			prizePool_,
291			claimer_,
292			yieldFeeRecipient_,
293			yieldFeePercentage_,
294			owner_
295		);
296	}

Here we can see in line #281 that the contract gives max approval to the yieldVault_ contract, which is supplied by the owner when creating a Vault, over the underlying asset of the vault. The Vault is created trough the VaultFactory, but that contract does not make any sanity check over the addresses passed in to the constructor.

This way, the malicious owner can craft a yieldVault_ contract with a rugpull function he can call anytime to drain all the funds from the vault and send them to him. And of course all the underlying assets deposited in the yieldVault_ contract could be drained too.

setLiquidationPair

665	  function setLiquidationPair(
666		    LiquidationPair liquidationPair_
667	  ) external onlyOwner returns (address) {
668		    if (address(liquidationPair_) == address(0)) revert LPZeroAddress();
669	
670		    IERC20 _asset = IERC20(asset());
671		    address _previousLiquidationPair = address(_liquidationPair);
672		
673		    if (_previousLiquidationPair != address(0)) {
674			      _asset.safeApprove(_previousLiquidationPair, 0);
675		    }
676		
677		    _asset.safeApprove(address(liquidationPair_), type(uint256).max);
678		
679		    _liquidationPair = liquidationPair_;
680		
681		    emit LiquidationPairSet(liquidationPair_);
682		    return address(liquidationPair_);
683	  }

Here is the same for the liquidationPair_ contract, which is given max approval in line #677. Same case as before, the address is supplied by the malicious owner of the vault, who can craft a contract with a rug pull function that transfers all the underlying assets to him.

Tools Used

Manual review

Recommended Mitigation Steps

A solution I can think of is checking the code of the deployed contracts in both the yieldVault_ and liquidationPair_ addresses against a set of whitelisted implementations of each contract. This would prevent the owner from setting maliciously crafted contracts that would allow him to rug pull the funds.

Assessed type

Rug-Pull