Tigris Trade contest - carlitox477's results

Lines of code

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

Vulnerability details

Description

The current implementation forces a user to extend their bonds for at least they current bond period. These mean that, for instance, a bond which was initially locked for 365 can never be extended, even after a week of being created.

If we consider that a bond should have at least a 7 days lock and at the most 365 days, then the current BondNFT.extendLock function should be refactored.

Impact

• Current BondNFT.extendLock function does not work as expected, forcing user who want to extend their bond to extend them at least for their current bond.period.
• For bonds which were set with a lock period of 365 days, they can not be extended, even after days of their creation.

POC

// In 09.Bond.js,  describe "Extending lock"
it("POC: Extending the lock does not work as expected", async function () {
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
      // user lock bond funds for 10 days
      await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 10);

      const fiveDaysTime = 5 * 24 * 60 * 60
      const eightDaysTime = 8 * 24 * 60 * 60

      // owner distribute rewards
      console.log("User created a lock for 10 days")
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("10"));
      await bond.connect(owner).distribute(stabletoken.address, ethers.utils.parseEther("10"));

      // Five days pass
      await network.provider.send("evm_increaseTime", [fiveDaysTime]); // Skip 10 days
      await network.provider.send("evm_mine");
      console.log("\n5 days pass")

      // User decide to extend their lock three days, given the current implementation the user is forced to extended 13 days
      const bondInfoBeforeExtension = await bond.idToBond(1)
      console.log(`Bond info before extension: {period: ${bondInfoBeforeExtension.period}, expireEpoch: ${bondInfoBeforeExtension.expireEpoch}}`)
      
      await lock.connect(user).extendLock(1, 0, 3)
      console.log("Bond was extended for 3 days")
      const bondInfoAfterExtension = await bond.idToBond(1)
      console.log(`Bond info after extension: {period: ${bondInfoAfterExtension.period}, expireEpoch: ${bondInfoAfterExtension.expireEpoch}}`)

      // 8 days pass, user should be able to release the bond given the extension of 3 days (8 days should be enough)
      await network.provider.send("evm_increaseTime", [eightDaysTime]);
      await network.provider.send("evm_mine");
      console.log("\n8 days later")
      console.log("After 13 days (10 original days + 3 days from extension) the user can not release the bond")
      
      // The user decide to claim their part and get their bond amount
      // The user should recieve all the current funds in the contract
      await expect(lock.connect(user).release(1)).to.be.revertedWith('!expire')

    });

Mitigation steps

In order to extendLock to work properly, the current implementation should be changed to:

function extendLock(
    uint _id,
    address _asset,
    uint _amount,
    uint _period,
    address _sender
) external onlyManager() {
    Bond memory bond = idToBond(_id);
    Bond storage _bond = _idToBond[_id];
    require(bond.owner == _sender, "!owner");
    require(!bond.expired, "Expired");
    require(bond.asset == _asset, "!BondAsset");
    require(bond.pending == 0); //Cannot extend a lock with pending rewards
+   uint currentEpoch = block.timestamp/DAY;
-   require(epoch[bond.asset] == block.timestamp/DAY, "Bad epoch");
    require(epoch[bond.asset] == currentEpoch, "Bad epoch");

+   uint pendingEpochs = bond.expireEpoch - currentEpoch;
+   uint newBondPeriod = pendingEpochs + _period;
+   //In order to respect min bond period when we extend a bon
+   // Next line can be omitted at discretion of the protocol and devs
+   // If it is omitted any created bond would be able to be extended always (except from those with period = 365)
+   require(newBondPeriod >= 7, "MIN PERIOD");

-    require(bond.period+_period <= 365, "MAX PERIOD");
+    require(newBondPeriod <= 365, "MAX PERIOD");
    
    unchecked {
-       uint shares = (bond.amount + _amount) * (bond.period + _period) / 365;
+       uint shares = (bond.amount + _amount) * newBondPeriod / 365;

-       uint expireEpoch = block.timestamp/DAY + bond.period + _period;
+       uint expireEpoch = currentEpoch + newBondPeriod;

        totalShares[bond.asset] += shares-bond.shares;
        _bond.shares = shares;
        _bond.amount += _amount;
        _bond.expireEpoch = expireEpoch;
        _bond.period += _period;
        _bond.mintTime = block.timestamp; 
-       _bond.mintEpoch = epoch[bond.asset];
+       _bond.mintEpoch = currentEpoch;
-       bondPaid[_id][bond.asset] = accRewardsPerShare[bond.asset][epoch[bond.asset]] * _bond.shares / 1e18;
+       bondPaid[_id][bond.asset] = accRewardsPerShare[bond.asset][currentEpoch] * _bond.shares / 1e18;
    }
    emit ExtendLock(_period, _amount, _sender,  _id);
}

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L110-L120

Vulnerability details

Description

Adding to many assets through BondNFT.addAsset can DOS Lock.claimGovFees function. This would DOS functions claim, claimDebt, lock and release from Lock contract too.

DOS release would lead to permanent freeze of user funds.

Impact

Giving current BondNFT contract does not allow to remove assets, if enough number of assets are added, claimGovFees will not be callable due to block gas limit. This can make claim, claimDebt, lock, release uncallable. If release can not be called, this means that users fund will be stuck forever in BondNFT contract.

POC

Considering there is no way to remove an asset from asset state variable from BondNFT:

1. BondNFT owner can add a large number of asset through addAsset function
2. If enough assets are added then claimGovFees will always revert:

// Lock contract
    function claimGovFees() public {
        // @audit Assets added to bondNFT are queried
        address[] memory assets = bondNFT.getAssets();

        // @audit Function caller has no control over assets variables, a large assets array can make the loop reach the block gas limit
        for (uint i=0; i < assets.length; i++) {
            uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
            IGovNFT(govNFT).claim(assets[i]);
            uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
            IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
            bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
        }
    }

1. BondNFT has no way to remove assets
2. If claimGovFees suffers from DOS, then claim, claimDebt, lock, releasesuffers from DOS too:

// Lock contract
function claim(
    uint256 _id
) public returns (address) {
    claimGovFees(); //@audit If this function suffer from DOS, then claim suffers from DOS too
    // rest of function...
}

// Lock contract
function claimDebt(
    address _tigAsset
) external {
    claimGovFees(); //@audit If this function suffer from DOS, then claimDebt suffers from DOS too
    // rest of function...
}

// Lock contract
function lock(
    address _asset,
    uint _amount,
    uint _period
) public {
    require(_period <= maxPeriod, "MAX PERIOD");
    require(_period >= minPeriod, "MIN PERIOD");
    require(allowedAssets[_asset], "!asset");

    claimGovFees(); //@audit If this function suffer from DOS, then lock suffers from DOS too

    // rest of function...
}

// Lock contract
function release(
    uint _id
) public {
    claimGovFees(); //@audit If this function suffer from DOS, then release suffers from DOS too
    (uint amount, uint lockAmount, address asset, address _owner) = bondNFT.release(_id, msg.sender);
    totalLocked[asset] -= lockAmount;
    IERC20(asset).transfer(_owner, amount);
}

1. Giving BondNFT.release is the only way for a user to recover bonds funds, and it is only callable by the manager, no user can recover their funds.
2. Neither BondNFT owner, nor its manager can remove assets
3. All users funds are stuck in the contract forever.

Mitigation steps

Giving that claiming governance fees from all assets seems to be a requirement for the expected behavior of the contract (Lock as well as BondNFT), consider adding a removeAsset function in BondNFT contract. Also, consider adding a max asset limit to add to BondNFT contract.

Lines of code

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

Vulnerability details

Description

When initiateMarketOrder is called, _marginAfterFees are calculated and then use it to calculate _positionSize

uint256 _marginAfterFees = _tradeInfo.margin - _handleOpenFees(_tradeInfo.asset, _tradeInfo.margin*_tradeInfo.leverage/1e18, _trader, _tigAsset, false);
uint256 _positionSize = _marginAfterFees * _tradeInfo.leverage / 1e18;

The problem is that _handleOpenFees does not consider referrer fees when it calculates its output (paidFees), leading to open a position greater than expected.

Impact

For a referred trade, initiateMarketOrder always opens a position greater than the one supposed, by allowing to use more margin than the one expected.

POC

The output of _handleOpenFees is _feePaid, which is calculated once, and it does not consider referralFees

// No refferal fees are considered
_feePaid =
    _positionSize
    * (_fees.burnFees + _fees.botFees) // get total fee%
    / DIVISION_CONSTANT // divide by 100%
    + _daoFeesPaid;

Then we can notice that, if the output of _handleOpenFees did not consider referral fees, neither would _marginAfterFees do

uint256 _marginAfterFees =
    _tradeInfo.margin-
    _handleOpenFees(
        _tradeInfo.asset,
        _tradeInfo.margin*_tradeInfo.leverage/1e18, 
        _trader,
        _tigAsset,
        false);

// @audit Then _positionSize would be greater than what is supposed to be, allowing to create a position greater than expected
uint256 _positionSize = _marginAfterFees * _tradeInfo.leverage / 1e18;

Mitigation steps

Consider referral fees when _feePaid is calculated in _handleOpenFees

// In _handleOpenFees function
+   uint256 _refFeesToConsider = _referrer == address(0) ? 0 : _fees.referralFees;
    _feePaid =
        _positionSize
-       * (_fees.burnFees + _fees.botFees) // get total fee%
+       * (_fees.burnFees + _fees.botFees + _refFeesToConsider) // get total fee%
        / DIVISION_CONSTANT // divide by 100%
        + _daoFeesPaid;