Dust will be accumulated when calculating the protocol and creator revenues due to the division performed

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

PredyPool will have tokens stuck because of a wrong protocol and creator fees. It will happen then the totalProtocolFee is not an even number.

Proof of Concept

Currently, the following functions trigger the interest update for the token pairs:

• PredyPool::supply
• PredyPool::execLiquidationCall
• PredyPool::getPairStatus
• PredyPool::revertPairStatus
• PredyPool::reallocate
• PredyPool::withdraw
• PredyPool::trade

All of them will accumulate huge amounts of locked tokens in the pool because the ApplyInterestLib::applyInterestForPoolStatus uses the wrong totalProtocolFee calculation:

ApplyInterestLib.sol

  function applyInterestForPoolStatus(Perp.AssetPoolStatus storage poolStatus, uint256 lastUpdateTimestamp, uint8 fee)
      internal
      returns (uint256 interestRate, uint256 totalProtocolFee)
  {
...MORE CODE
      totalProtocolFee = poolStatus.tokenStatus.updateScaler(interestRate, fee);

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

As we can see the intention is to have equally distributed fees to the operator and owner, but in reality small amounts of tokens will always be left in the contract, without a way to be retrieved when totalProtocolFee is an odd number.

For example let’s say the total fee is 1111 tokens, then both users will receive 555 tokens and the rest 1 token will be left in the contract.

This might seem a negligible number, but given that all the functions above are using the wrong calculation, in a short amount of time a lot of tokens will be left in the PredyPool without a way to be claimed by anyone, especially when there is a high activity.

Important to note that this will happen for both quote and base tokens.

Even more, as the time passes and PredyPool continues to accumulate dust, reserves will also be highly increased.

Similar issue:

https://solodit.xyz/issues/native-tokens-permanently-stuck-in-strategypassivemanageruniswap-contract-due-to-rounding-in-_chargefees-cyfrin-none-cyfrin-beefy-finance-markdown

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the function to not leave locked tokens.

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);
+     uint256 protocolRevenue = totalProtocolFee / 2; 

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

Assessed type

Math

[syuhei176]

Since low decimal tokens are out of scope, I don't think this is high risk.

[c4-sponsor]

@syuhei176 Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[c4-sponsor]

@syuhei176 Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[alex-ppg]

The Warden and its duplicate specify that a truncation issue will occur for uneven numbers in the ApplyInterestLib::applyInterestForPoolStatus function.

As the Sponsor correctly states, the maximum impact of this submission is two wei, and such a value will be negligible for all intents and purposes. While a valid recommendation, its severity level is better suited as QA (NC).

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c

[blckhv]

Hey @alex-ppg,

I would like to add some clarifications to this report.
The sponsor mentioned that low decimal tokens are out of scope but indeed wBTC is one of the tokens that will be used in the protocol - here.

Another thing is the maximum impact and why it is not two wei, but more.
These two wei are per transaction but since all the entry point in the system will end up calling the applyInterestForPoolStatus function it will continuously accumulate locked tokens in the Pool contract.

Given these 2 preconditions, I think the impact can fall into this category from C4 documentation:
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements., which additionally complies with the initial intention of the sponsor to add the Med Risk label

Thanks!

[alex-ppg]

Hey @blckhv, thanks for contributing to the PJQA process! The value lost here is negligible and would require millions of transactions to occur before it is an observable value (i.e. 1m transactions would lead to 500 USD in value if we consider WBTC only being transacted).

For all intents and purposes, this is a QA (L) risk flaw.