Malicious vault owner can drain all users assets from the vault

[code423n4]

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

[c4-judge]

Picodes marked the issue as duplicate of #324

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory