Email address

mail@cmichel.io

Handle

@cmichelio

Eth address

0x6823636c2462cfdcD8d33fE53fBCD0EdbE2752ad

Vulnerability details

Attacker calls MarginRouter.crossSwapExactTokensForTokens with a fake pair and the same token[0] == tokne[1]. crossSwapExactTokensForTokens(1000 WETH, 0, [ATTACKER_CONTRACT], [WETH, WETH]). When the amounts are computed by the amounts = UniswapStyleLib.getAmountsOut(amountIn - fees, pairs, tokens); call, the attacker contract returns fake reserves that yield 0 output When _swapExactT4T is called, the funds are sent to the fake contract and doing nothing passes all checks in _swap call that follows because the startingBalance is stored after the initial Fund withdraw to the pair.

function _swapExactT4T() {
  // withdraw happens here
    Fund(fund()).withdraw(tokens[0], pairs[0], amounts[0]);
    _swap(amounts, pairs, tokens, fund());
}

function _swap() {
  uint256 startingBalance = IERC20(outToken).balanceOf(_to);
  uint256 endingBalance = IERC20(outToken).balanceOf(_to);
  // passes as startingBalance == endingBalance + 0
  require(
      endingBalance >= startingBalance + amounts[amounts.length - 1],
      "Defective AMM route; balances don't match"
  );
}

Impact

The full impact is not yet known as registerTrade could still fail when subtracting the inAmount and adding 0 outAmount. At least, this attack is similar to a withdrawal which is supposed to only occur after a certain coolingOffPeriod has passed, but this time-lock is circumvented with this attack.

Recommended mitigation steps

Move the fund withdrawal to the first pair after the startingBalance assignment. Check fromToken != toToken as cyclical trades (arbitrages) are likely not what margin traders are after. Consider if the same check is required for registerTradeAndBorrow / adjustAmounts functions.

code-423n4 / 2021-04-marginswap-findings