Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c4-bot-4 opened this issue Jun 11, 2024 · 0 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 🤖_primary AI based primary recommendation 🤖_83_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-4
Copy link
Contributor

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

@c4-bot-4 c4-bot-4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 11, 2024
c4-bot-4 added a commit that referenced this issue Jun 11, 2024
@c4-bot-12 c4-bot-12 added 🤖_83_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Jun 14, 2024
howlbot-integration bot added a commit that referenced this issue Jun 17, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 🤖_primary AI based primary recommendation 🤖_83_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants