One can call the MarginRouter.crossSwapExactTokensForTokens function first with a fake contract disguised as a token pair:
crossSwapExactTokensForTokens(0.0001 WETH, 0, [ATTACKER_CONTRACT], [WETH, WBTC]). When the amounts are computed by the amounts = UniswapStyleLib.getAmountsOut(amountIn - fees, pairs, tokens); call, the attacker contract returns fake reserves that yield 1 WBTC for the tiny input.
The resulting amount is credited through registerTrade.
Afterwards, _swapExactT4T([0.0001 WETH, 1 WBTC], 0, [ATTACKER_CONTRACT], [WETH, WBTC]) is called with the fake pair and token amounts.
At some point _swap is called, the starting balance is stored in startingBalance, and the attacker contract call allows a re-entrancy:
pair.swap(0.0001 WETH, 1 WBTC, FUND, new bytes(0)); // can re-enter here
From the ATTACKER_CONTRACT we re-enter the MarginRouter.crossSwapExactTokensForTokens(30 WETH, 0, WETH_WBTC_PAIR, [WETH, WBTC]) function with the actual WETH <> WBTC pair contract.
All checks pass, the FUND receives the actual amount, the outer _swap continues execution after the re-entrancy and the endingBalance >= startingBalance + amounts[amounts.length - 1] check passes as well because the inner swap successfully deposited these funds.
We end up doing 1 real trade but being credited twice the output amount.
Impact
This allows someone to be credited multiples of the actual swap result. This can be repeated many times and finally, all tokens can be stolen.
Recommended mitigation steps
Add re-entrancy guards (from OpenZeppelin) to all external functions of MarginRouter.
There might be several attack vectors of this function as the attacker controls many parameters.
The idea of first doing an estimation with UniswapStyleLib.getAmountsOut(amountIn - fees, pairs, tokens) and updating the user with these estimated amounts, before doing the actual trade, feels quite vulnerable to me.
Consider removing the estimation and only doing the actual trade first, then calling registerTrade with the actual trade amounts returned.
Email address
mail@cmichel.io
Handle
@cmichelio
Eth address
0x6823636c2462cfdcD8d33fE53fBCD0EdbE2752ad
Vulnerability details
One can call the
MarginRouter.crossSwapExactTokensForTokensfunction first with a fake contract disguised as a token pair:crossSwapExactTokensForTokens(0.0001 WETH, 0, [ATTACKER_CONTRACT], [WETH, WBTC]). When the amounts are computed by theamounts = UniswapStyleLib.getAmountsOut(amountIn - fees, pairs, tokens);call, the attacker contract returns fake reserves that yield 1 WBTC for the tiny input. The resulting amount is credited throughregisterTrade. Afterwards,_swapExactT4T([0.0001 WETH, 1 WBTC], 0, [ATTACKER_CONTRACT], [WETH, WBTC])is called with the fake pair and token amounts. At some point_swapis called, the starting balance is stored instartingBalance, and the attacker contract call allows a re-entrancy:From the ATTACKER_CONTRACT we re-enter the
MarginRouter.crossSwapExactTokensForTokens(30 WETH, 0, WETH_WBTC_PAIR, [WETH, WBTC])function with the actual WETH <> WBTC pair contract. All checks pass, the FUND receives the actual amount, the outer_swapcontinues execution after the re-entrancy and theendingBalance >= startingBalance + amounts[amounts.length - 1]check passes as well because the inner swap successfully deposited these funds. We end up doing 1 real trade but being credited twice the output amount.Impact
This allows someone to be credited multiples of the actual swap result. This can be repeated many times and finally, all tokens can be stolen.
Recommended mitigation steps
Add re-entrancy guards (from OpenZeppelin) to all external functions of
MarginRouter.There might be several attack vectors of this function as the attacker controls many parameters. The idea of first doing an estimation with
UniswapStyleLib.getAmountsOut(amountIn - fees, pairs, tokens)and updating the user with these estimated amounts, before doing the actual trade, feels quite vulnerable to me. Consider removing the estimation and only doing the actual trade first, then callingregisterTradewith the actual trade amounts returned.