Liquidator can liquidate the max amout of _liquidableYield for as low as 1 wei of PrizeTokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

• The reward mechanism won't work as expected because the PrizePool won't be filled with the expected amount of PrizeTokens that the liquidator should contribute to the PrizePool when liquidating the yield fees

• Liquidator can get away with all the yield fees without contributing the equivalent amount of PrizeTokens to the PrizePool contract

Proof of Concept

• This issue is caused because the vault's owner can set the liquidator address to be any arbitrary address, instead of deploying a new Liquidator contract and assign it as the liquidator of the vault.

  • If the liquidator can be any address, it can call the Vault::liquidate() and pass 1 wei as the _amountIn parameter, in the function's execution it will contribute to the PrizePool the exact received amount of _amounIn PrizeTokens.

  • The expected behavior is that the Liquidator contract will compute in advance how many PrizeTokens must be contributed to the PrizePool in exchange of liquidating the _amountOut up to the maximum available yield fees, but if the liquidator is set to be an address of a contract other than the real Liquidator contract, then the vault's owner can use this fake Liquidator contract to liquidate the max available yield fee for as low as 1 wei of PrizeToken.

  • This will cause that the PrizePool will never receive any PrizeTokens for further distribution among the Vault depositors.

  • Liquidator will take away all the yield fee that was generated using the Vault depositor's assets and they won't receive any reward from having deposited their assets

  • Basically the core purpose of the PoolTogether Vaults will be broken and PrizePool won't receive any PrizeTokens to distribute to the Vault Depositors

Tools Used

Manual Audit

Recommended Mitigation Steps

• The recommendation is to not allow vault owners to set the Liquidator to be an arbitrary address, if the protocol is creating a contract for the Liquidators, then, force the Vaults to use such a contract.

  • On the Vault:setLiquidatonPair(), remove the liquidationPair_ parameter and instead deploy a new contract and assing the address of the newly deployed Liquidator as the _liquidationPair of for the Vault.

    
    function setLiquidationPair(

• LiquidationPair liquidationPair_ ) external onlyOwner returns (address) {

• if (address(liquidationPair_) == address(0)) revert LPZeroAddress();

  IERC20 _asset = IERC20(asset()); address _previousLiquidationPair = address(_liquidationPair);

  if (_previousLiquidationPair != address(0)) { _asset.safeApprove(_previousLiquidationPair, 0); }

• _liquidationPair = new LiquidationPair();

  asset.safeApprove(address(liquidationPair), type(uint256).max);

• liquidationPair = liquidationPair;

  emit LiquidationPairSet(liquidationPair); return address(liquidationPair); }

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #376

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope

[stalinMacias]

Hey @Picodes As per the sponsor's comment about this issue, what I'm reporting is not the fact that the LiquidationPair contract is a trusted free bugs contract, what I'm reporting is the fact that it is possible to set the address of the LiquidationPair as any arbitrary contract, from which it can be liquidated the yield for as low as 1 wei, is just a matter of directly calling the Vault::liquidate() function from the fake LiquidationPair

And what I'm suggesting indeed matches the sponsor's comment about assuming that the LiquidationPair is a trusted contract, because when setting a new LiquidationPair it won't be possible to pass any arbitrary address, but instead, the same contract will create a new LiquidationPair using the expected code for the LiquidationPairs.

[Picodes]

@stalinMacias fair point. I'll flag this issue as a duplicate of #300 which gathers all issues about malicious owner behaviors.

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #300

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)