3 ms·
Comment clarification: Ethermine is still at it, and still screwing everybody over. I don't like malicious contracts, but, I'm not sure I totally disagree with
by jl2718 6y ago
Comment clarification:
Ethermine is still at it, and still screwing everybody over. I don't like malicious contracts, but, I'm not sure I totally disagree with vigilante justice when nothing else will stop them. I wouldn't touch those Eth in the pool though.
One thing I didn't know was that the UniSwapV2 Router is still vulnerable to this because they have functions like this:
function swapExactETHForTokens(uint amountOutMin, address[] calldata path, address to, uint deadline)
external
virtual
override
payable
ensure(deadline)
returns (uint[] memory amounts)
{
require(path[0] == WETH, 'UniswapV2Router: INVALID_PATH');
amounts = UniswapV2Library.getAmountsOut(factory, msg.value, path);
require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');
IWETH(WETH).deposit{value: amounts[0]}();
assert(IWETH(WETH).transfer(UniswapV2Library.pairFor(factory, path[0], path[1]), amounts[0]));
_swap(amounts, path, to);
}
The problem here is that the swap amounts are determined at execution time versus entry time. The pools themselves are entry-time trades, and so front-running would just cancel their trade.
function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external lock {
require(amount0Out > 0 || amount1Out > 0, 'UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT');
(uint112 _reserve0, uint112 _reserve1,) = getReserves(); // gas savings
require(amount0Out < _reserve0 && amount1Out < _reserve1, 'UniswapV2: INSUFFICIENT_LIQUIDITY');
uint balance0;
uint balance1;
{ // scope for _token{0,1}, avoids stack too deep errors
address _token0 = token0;
address _token1 = token1;
require(to != _token0 && to != _token1, 'UniswapV2: INVALID_TO');
if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
if (data.length > 0) IUniswapV2Callee(to).uniswapV2Call(msg.sender, amount0Out, amount1Out, data);
balance0 = IERC20(_token0).balanceOf(address(this));
balance1 = IERC20(_token1).balanceOf(address(this));
}
uint amount0In = balance0 > _reserve0 - amount0Out ? balance0 - (_reserve0 - amount0Out) : 0;
uint amount1In = balance1 > _reserve1 - amount1Out ? balance1 - (_reserve1 - amount1Out) : 0;
require(amount0In > 0 || amount1In > 0, 'UniswapV2: INSUFFICIENT_INPUT_AMOUNT');
{ // scope for reserve{0,1}Adjusted, avoids stack too deep errors
uint balance0Adjusted = balance0.mul(1000).sub(amount0In.mul(3));
uint balance1Adjusted = balance1.mul(1000).sub(amount1In.mul(3));
require(balance0Adjusted.mul(balance1Adjusted) >= uint(_reserve0).mul(_reserve1).mul(1000**2), 'UniswapV2: K');
}
_update(balance0, balance1, _reserve0, _reserve1);
emit Swap(msg.sender, amount0In, amount1In, amount0Out, amount1Out, to);
}
It's quite surprising to me that the router allows this, and the interface uses the unsafe function, when the safer swap() function is available on the pool, and the math is already done in the browser. Seems like a huge waste of gas too.
And lastly, I don't expect this trade re-ordering problem to go away with Eth2. It could get a lot worse because there is no need to coordinate across a huge number of independent worker nodes in the pool.