Skip to content

Latest commit

 

History

History
108 lines (81 loc) · 4.1 KB

File metadata and controls

108 lines (81 loc) · 4.1 KB

Tangy Tortilla Fox

Medium

sellVotes slippage protection is insufficient

Summary

sellVotes does not take into account the fees from the trade, getting a worse price for the user, even though he had slippage protection.

https://github.com/sherlock-audit/2024-12-ethos-update/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L539

  function sellVotes(
    uint256 profileId,
    bool isPositive,
    uint256 votesToSell,
    uint256 minimumVotePrice
  ) public whenNotPaused activeMarket(profileId) nonReentrant {
    _checkMarketExists(profileId);
    (uint256 proceedsBeforeFees, uint256 protocolFee, uint256 proceedsAfterFees) = _calculateSell(
      markets[profileId],
      profileId,
      isPositive,
      votesToSell
    );

    uint256 pricePerVote = votesToSell > 0
      ? proceedsBeforeFees / votesToSell
      : 0;

    // We are comparing before the fees, should it be that way ?
    // uniswap is checking for after fee
    if (pricePerVote < minimumVotePrice) {
      revert SellSlippageLimitExceeded(minimumVotePrice, pricePerVote);
    }

This will lead to users getting worse trades than their slippage protection, due to the fee not being accounted for. Most protocols include their fee inside their slippage protection. For example if we look into uniswap we find out that that's the case there: https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L224

    function swapExactTokensForTokens(... ) external virtual override ensure(deadline) returns (uint[] memory amounts) {
        amounts = UniswapV2Library.getAmountsOut(factory, amountIn, path); // getAmountsOut used for slippage
        require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');

https://github.com/Uniswap/v2-periphery/blob/master/contracts/libraries/UniswapV2Library.sol#L62-L70

    // performs chained getAmountOut calculations on any number of pairs
    function getAmountsOut(address factory, uint amountIn, address[] memory path) internal view returns (uint[] memory amounts) {
        require(path.length >= 2, 'UniswapV2Library: INVALID_PATH');
        amounts = new uint[](path.length);
        amounts[0] = amountIn;
        for (uint i; i < path.length - 1; i++) {
            (uint reserveIn, uint reserveOut) = getReserves(factory, path[i], path[i + 1]);
            amounts[i + 1] = getAmountOut(amounts[i], reserveIn, reserveOut);
        }
    }

    function getAmountOut(uint amountIn, uint reserveIn, uint reserveOut) internal pure returns (uint amountOut) {
        require(amountIn > 0, 'UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT');
        require(reserveIn > 0 && reserveOut > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY');
        uint amountInWithFee = amountIn.mul(997); // We calculate the amountOut while including the fee
        uint numerator = amountInWithFee.mul(reserveOut);
        uint denominator = reserveIn.mul(1000).add(amountInWithFee);
        amountOut = numerator / denominator;
    }

Note that this is opposite to buy's price, as it's slippage taken into account the total amount - funds spend + fees paid:

https://github.com/sherlock-audit/2024-12-ethos-update/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L448-L449

    (, , , uint256 total) = _calculateBuy(markets[profileId], isPositive, minVotesToBuy);
    if (total > msg.value) revert InsufficientFunds();

Root Cause

minimumVotePrice not accounting the fees from the trade, resulting in worse price for the user.

Internal Pre-conditions

No response

External Pre-conditions

No response

Attack Path

  1. User sells some votes with slippage protection of 10%
  2. Market price moves the price down 10% (but the TX passes)
  3. However since the fees are also 10%, he actually gets 90% * 10% = 9% 19% value diff

Our user had 10% slippage protection on his price, but sill lost almost 20% of his value (2 times more than his slippage).

Impact

Users lose funds Slippage protection is insufficient

PoC

No response

Mitigation

Account the slippage after taking the fees.