044.md

Ambitious Cotton Hyena

Medium

Slippage Protection on Sell votes is implemented incorrectly

Summary

The Slippage Protection uses proceedsBeforeFees instead of proceedsAfterFees, which makes user get paid for vote with price less than intended

Description

When users sell there votes they provide a minimum price per vote to protect themselves from slippage and price changes. This is made by getting the number of votes that the user will sell, and then divide the total price of them to the number of the votes he sold, to represent an average price of each vote. Then this value is compared against the one provided by the user.

The problem is that when selling votes, there is an exit fees is taken from the amount we took so proceedsBeforeFees is greater than proceedsAfterFees in that case, and the user takes proceedsAfterFees amount.

ReputationMarket::sellVotes() ReputationMarket.sol#L553-L556

  function sellVotes( ... ) public whenNotPaused activeMarket(profileId) nonReentrant {
    _checkMarketExists(profileId);
    (uint256 proceedsBeforeFees, uint256 protocolFee, uint256 proceedsAfterFees) = _calculateSell( ... );

>>  uint256 pricePerVote = votesToSell > 0 ? proceedsBeforeFees / votesToSell : 0;
    if (pricePerVote < minimumVotePrice) {
      revert SellSlippageLimitExceeded(minimumVotePrice, pricePerVote);
    }
// -------------------------------
  function _calculateSell( ... ) ... {
    ...
    proceedsBeforeFees = _calcCost(market, isPositive, false, votesToSell);
>>  (proceedsAfterFees, protocolFee) = previewExitFees(proceedsBeforeFees);
  }
// -------------------------------
  function previewExitFees(
    uint256 proceedsBeforeFees
  ) private view returns (uint256 totalProceedsAfterFees, uint256 protocolFee) {
    protocolFee = (proceedsBeforeFees * exitProtocolFeeBasisPoints) / BASIS_POINTS_BASE;
>>  totalProceedsAfterFees = proceedsBeforeFees - protocolFee;
  }

proceedsBeforeFees represents the total amount we will subtract from the market, but proceedsAfterFees represent the exact amount the user will take. And in the slippage check we are comparing the value against proceedsBeforeFees which will make the price of each votes gets calculated with a value greater than the value that will be given to the user.

Proof of Concept

• userA bought 10 Votes
• userA wants to sell these 10 votes where each vote should give him at least 100
• userA fired sellVotes and made minimumVotePrice equals 100
• Votes are getting Sold
• Protoolc Exit Fees are 3%
• Decreasing these 10 votes gives the total amount of 1000. i.e: proceedsBeforeFees` equals 1000
• proceedsAfterFees will be proceedsBeforeFees * 3% = 970
• Doing a Slippage check uses proceedsBeforeFees which is 1000 so 1000 / 10 equals 100 which makes the slippage check passes
• Sending the money for that 10 votes to the user by sending him proceedsAfterFees which is 970
• The actual average price of the vote the user takes is 970 / 10 i.e 97 instead of 100
• The user takes less than he intended because the slippage check considers protocol fees too.

Recommendations

Implement the slippage check using proceedsAfterFees instead of proceedsBeforeFees

diff --git a/ethos/packages/contracts/contracts/ReputationMarket.sol b/ethos/packages/contracts/contracts/ReputationMarket.sol
index 8590a90..3ef8f10 100644
--- a/ethos/packages/contracts/contracts/ReputationMarket.sol
+++ b/ethos/packages/contracts/contracts/ReputationMarket.sol
@@ -550,7 +550,7 @@ contract ReputationMarket is AccessControl, UUPSUpgradeable, ReentrancyGuard, IT
       votesToSell
     );
 
-    uint256 pricePerVote = votesToSell > 0 ? proceedsBeforeFees / votesToSell : 0;
+    uint256 pricePerVote = votesToSell > 0 ? proceedsAfterFees / votesToSell : 0;
     if (pricePerVote < minimumVotePrice) {
       revert SellSlippageLimitExceeded(minimumVotePrice, pricePerVote);
     }