Treasury.buybackRnbw() is vulnerable to price manipulation attacks

[andreiashu]

Description

The owner of the Treasury can call the buybackRnbw function to convert one or more of the underlying tokens within a lending pool to rnbw tokens:

review-xave-lending-market-2021-10/code/contracts/buyback/Treasury.sol

Line 44 in fbfed0a

function buybackRnbw(address[] calldata _underlyings) external onlyOwner returns (uint256) {

The function first uses the cloned DFX protocol to convert the token into USDC:

review-xave-lending-market-2021-10/code/contracts/buyback/Treasury.sol

Lines 81 to 88 in fbfed0a

	uint256 targetAmount =
	ICurve(curveAddress).originSwap(
	_underlying,
	USDC,
	_underlyingAmount,
	0,
	block.timestamp + 60
	);

After that the Uniswap V2 protocol is used to convert from USDC to RNBW tokens:

review-xave-lending-market-2021-10/code/contracts/buyback/Treasury.sol

Lines 59 to 69 in fbfed0a

	address[] memory path = new address[](3);
	path[0] = USDC;
	path[1] = WETH9;
	path[2] = rnbw;
	rnbwBought = IUniswapV2Router02(router).swapExactTokensForTokens(
	usdcBalance,
	0,
	path,
	address(this),
	block.timestamp + 60
	)[0];

The issue is that in both token swap cases above, the arguments amountOutMin (for Uniswap) and _minTargetAmount (for DFX Curve contract) are passed as 0 values. This means that the Treasury contract does not enforce any minimum amount expected for the output of RNBW tokens swapped.

The reason why the above two implementations are vulnerable to price manipulation is explained in the Uniswap V2 Safety Considerations section:

Because Ethereum transactions occur in an adversarial environment, smart contracts that do not perform safety checks can be exploited for profit. If a smart contract assumes that the current price on Uniswap is a "fair" price without performing safety checks, it is vulnerable to manipulation. A bad actor could e.g. easily insert transactions before and after the swap (a "sandwich" attack) causing the smart contract to trade at a much worse price, profit from this at the trader's expense, and then return the contracts to their original state. (One important caveat is that these types of attacks are mitigated by trading in extremely liquid pools, and/or at low values.)

Recommendation

The best way to protect against these attacks is to use an external price feed or "price oracle". The best "oracle" is simply traders' off-chain observation of the current price, which can be passed into the trade as a safety check.

The buybackRnbw function can accept an additional parameter minRNBWAmount that can be checked after the two above steps are performed, or passed to the swapExactTokensForTokens Uniswap function, to ensure that an expected minimum amount of RNBW tokens were received by the Treasury contract.

For example, Uniswap V2 getAmountsOut can be used by a frontend to calculate a fair value for USDC / RNBW:

Given an input asset amount and an array of token addresses calculates all subsequent maximum output token amounts by calling getReserves for each pair of token addresses in the path in turn, and using these to call getAmountOut.

Useful for calculating optimal token amounts before calling swap.

References

Uniswap V2 Documentation: Implement a Swap

DEFI Sandwich Attack Explaination

Rapid Rise of MEV in Ethereum