updatePoolOwner should call withdrawCreatorRevenue, otherwise, creator fees will be given to the next owner

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/ApplyInterestLib.sol#L50-L73
https://github.com/code-423n4/2024-05-predy/blob/main/src/PredyPool.sol#L157-L159

Vulnerability details

Impact

Changing the pool owner without first invoking the PredyPool::withdrawCreatorRevenue admin will make him lose all of the accumulated fees.

The issue is even worse because not only the Predy team will be able to create new pairs and it is not expected from the normal users to know that they should explicitly claim their fees, before changing the owner.

Proof of Concept

All actions in the Predy system invoke the ApplyInterestLib::applyInterestForToken function which is responsible for updating the fee and premium growth as well as the interest and most importantly for our issue: accruing accumulatedProtocolRevenue and accumulatedCreatorRevenue that belong to the pool operator and pool owner respectively, so in pools with a high activity they will be accruing a lot of funds to belonging to their claimers.

ApplyInterestLib.sol

function applyInterestForPoolStatus(Perp.AssetPoolStatus storage poolStatus, uint256 lastUpdateTimestamp, uint8 fee)
      internal
      returns (uint256 interestRate, uint256 totalProtocolFee)
  {
      if (block.timestamp <= lastUpdateTimestamp) {
          return (0, 0);
      }

      uint256 utilizationRatio = poolStatus.tokenStatus.getUtilizationRatio();

      // Skip calculating interest if utilization ratio is 0
      if (utilizationRatio == 0) {
          return (0, 0);
      }

      // Calculates interest rate
      interestRate = InterestRateModel.calculateInterestRate(poolStatus.irmParams, utilizationRatio)
          * (block.timestamp - lastUpdateTimestamp) / 365 days;

      totalProtocolFee = poolStatus.tokenStatus.updateScaler(interestRate, fee);

      poolStatus.accumulatedProtocolRevenue += totalProtocolFee / 2; 
      poolStatus.accumulatedCreatorRevenue += totalProtocolFee / 2;
  }

At any time these fees can be claimed from PredyPool::withdrawProtocolRevenue and PredyPool::withdrawCreatorRevenue.

The issue is that the pool owner have the ability to change the pool ownership and give it to another user, but it is performed without first claiming the fees from PredyPool::withdrawCreatorRevenue.

PredyPool.sol

   function updatePoolOwner(uint256 pairId, address poolOwner) external onlyPoolOwner(pairId) {
      AddPairLogic.updatePoolOwner(globalData.pairs[pairId], poolOwner);
  }

function updatePoolOwner(DataType.PairStatus storage _pairStatus, address _poolOwner) external {
      validatePoolOwner(_poolOwner);

      _pairStatus.poolOwner = _poolOwner;

      emit PoolOwnerUpdated(_pairStatus.id, _poolOwner);
  }

The impact is that the pool owners will be losing all of their designated creator fees if they do not manually call the function to claim their fees, which will lead to loss.

Tools Used

Manual Review

Recommended Mitigation Steps

Invoke PredyPool::withdrawCreatorRevenue in the updatePoolOwner function.

Assessed type

Governance

[blckhv]

Hey @alex-ppg,

I would like to bring your attention to this issue as it talks about loss of fees for the creators and the mechanics are similar to 134 from the findings repo.
Additionally there is no mention in both README and code that this should happen and there is no assumption that changing the creator without first claiming the fees should give them to the next creator.

There is yet another similar issue:
code-423n4/2024-05-munchables-findings#20

Thanks for your time!

[alex-ppg]

Hey @blckhv, thanks for contributing to the PJQA process! This represents a validation repository finding and as such was not evaluated by me directly.

The precedence cited (which I was a judge of) does not bear any resemblance to the issue at hand.

The issue described cannot be considered an HM vulnerability and would be a nice-to-have optional feature as it may well be desirable to permit the new owner to withdraw revenue.