Tangy Tortilla Fox
Medium
sellVotes
does not take into account the fees from the trade, getting a worse price for the user, even though he had slippage protection.
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:
(, , , uint256 total) = _calculateBuy(markets[profileId], isPositive, minVotesToBuy);
if (total > msg.value) revert InsufficientFunds();
minimumVotePrice
not accounting the fees from the trade, resulting in worse price for the user.
No response
No response
- User sells some votes with slippage protection of 10%
- Market price moves the price down 10% (but the TX passes)
- 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).
Users lose funds Slippage protection is insufficient
No response
Account the slippage after taking the fees.