Tigris Trade contest - 0xdeadbeef0x's results

Lines of code

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

Vulnerability details

Impact

Users who created a lock with a tigAsset and that tigAsset gets blacklisted permanently in BondNFT:

1. Will not be able to release their lock and receive their funds. (Loss of funds)
2. Will not be able to receive rewards for their stake. (Loss of yield)

Essentially, blacklisting a tigAsset results in lose of users staked funds and not being able to get expected rewards.

Any use of setAllowedAsset to set a tigAsset to not be allowed will result the above on user stakes of tigAssets.

Simple scenario:

1. User stakes X tigAssets into lock for 365 days.
2. After 5 days setAllowedAsset is called to set tigAsset to false in allowedAsset.
3. Users tigAssets are locked in the contract forever, with only receiving rewards for 5 days.

Users should be able to release their lock if the asset that they staked got blacklisted.

Proof of Concept

BondNFT has an allowedAsset list which is used to control what tigAssets are used for staking. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L63

    function createLock(
        address _asset,
        uint _amount,
        uint _period,
        address _owner
    ) external onlyManager() returns(uint id) {
        require(allowedAsset[_asset], "!Asset");
-----
    }

The tigAsset is always set to true in the allowedAsset when it is added as a supported asset. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L353

    function addAsset(address _asset) external onlyOwner {
----
        allowedAsset[_asset] = true;
-----
 }

The BondNFT contract also has a mechanism to "blacklist" an asset by calling the setAllowedAsset function: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L359

    function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
        require(assets[assetsIndex[_asset]] == _asset, "Not added");
        allowedAsset[_asset] = _bool;
    }

In most operations in the lock contract, it claims rewards from the govNFT contract and calls the distribute function in BondNFT to distribute the rewards to stakers.

The bug exists in the distribute function which returns false if the token is blacklisted: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L215

    function distribute(
        address _tigAsset,
        uint _amount
    ) external {
        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;
-----
                    epoch[_tigAsset] += 1;
-----
    }

Please not the following:

1. epoch[_tigAsset] will never get updated if the tigAsset is blacklisted.
2. distribute does not add the retrieved tokens from the govNFT to rewards accounting.

Consequence of #1: users cannot receive rewards for their stake.

Consequence of #2: users will never be able to release their lock even after the lock period has passed. This is because release function will revert with !expire message because the contract decides if a bond is expired or not according to epoch[_tigAsset].

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

    function release(
        uint _id,
        address _releaser
    ) external onlyManager() returns(uint amount, uint lockAmount, address asset, address _owner) {
        Bond memory bond = idToBond(_id);
        require(bond.expired, "!expire");
------
    }

idToBond in BondNFT that updates bond.expire: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L238

    function idToBond(uint256 _id) public view returns (Bond memory bond) {
------
        bond.expired = bond.expireEpoch <= epoch[bond.asset] ? true : false;
------
    }

As can be seen above because epoch[bond.asset] is not updated (due to blacklisting of the asset), the bond is considered not expired.

Hardhat POC

The protocol has already implemented a test that makes sure if a token is blacklisted it is not distributed: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/test/09.Bonds.js#L163

    it("Distributing an unallowed asset should return", async function () {
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("3000"));
      await lock.connect(owner).lock(StableToken.address, ethers.utils.parseEther("3000"), 365);
      await bond.connect(owner).setAllowedAsset(stabletoken.address, false);
      await govnft.connect(owner).distribute(stabletoken.address, ethers.utils.parseEther("1000"));
      await lock.connect(owner).claimGovFees();
      expect(await bond.pending(1)).to.be.equals(0);
    });

In the above test, 3000 tigAssets are staked in the lock and the staker will not receive any rewards.

The following POC demonstrates that a user cannot unlock his stake after the 100 days lock period: Add the following test to 09.Bonds.js: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/test/09.Bonds.js#L170

    it("Attempting to release a bond of an unallowed asset reverts", async function () {
      
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("3000"));
      await lock.connect(owner).lock(StableToken.address, ethers.utils.parseEther("3000"), 100);
      await bond.connect(owner).setAllowedAsset(stabletoken.address, false);

      await network.provider.send("evm_increaseTime", [8726400]); // Skip 101 days
      await network.provider.send("evm_mine");

      await expect(lock.connect(owner).release(1)).to.be.revertedWith("!expire"); // validate revert 
    });

To run the above test, execute:

npx hardhat test --grep "Attempting to release a bond of an unallowed asset reverts"

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

While there must be a need to blacklist tigAssets because setAllowedAsset function exists, the BondNFT contract should allow users to release their stake if the token they staked is blacklisted or automatically do it for them.

Lines of code

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

Vulnerability details

Impact

BondNFTs should be transferrable. According the the proposal and the sponsor, BondNFTs should could be sold and borrowed against. The proposal for context: https://gov.tigris.trade/#/proposal/0x2f2d1d63060a4a2f2718ebf86250056d40380dc7162fb4bf5e5c0b5bee49a6f3

The current implementation limits selling/depositing to only the same day that rewards are distributed for the tigAsset of the bond.

The impact if no rewards are distributed in the same day:

1. BondNFTs listed on open markets will not be able to fulfil the orders
2. BondNFTs deposited as collateral will not be release the collateral

Because other market/platforms used for selling/depositing will not call claimGovFees to distribute rewards, they will revert when trying to transfer the BondNFT.

Realistic examples could be BondNFTs listed on opensea.

Example of reasons why rewards would not be distributed in the same day:

1. Low activity from investors, rewards are distirbuted when users lock/release/extend
2. tigAsset is blacklisted in BondNFT, rewards will not be distributed in such case.

Proof of Concept

BondNFT has a mechanism to update the time tigAsset rewards are distributed. It uses a map that points to the last timestamp rewards were distributed for epoch[tigAsset].

distribute function in BondNFT: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L221

    function distribute(
        address _tigAsset,
        uint _amount
    ) external {
        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;
        IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount);
        unchecked {
            uint aEpoch = block.timestamp / DAY;
            if (aEpoch > epoch[_tigAsset]) {
                for (uint i=epoch[_tigAsset]; i<aEpoch; i++) {
                    epoch[_tigAsset] += 1;
                    accRewardsPerShare[_tigAsset][i+1] = accRewardsPerShare[_tigAsset][i];
                }
            }
            accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];
        }
        emit Distribution(_tigAsset, _amount);
    }

(Please not that if the asset is blacklisted through allowedAsset the epoch[tigAsset] will not be updated)

When BondNFTs are transfered, a check is implemented to make sure epoch[tigAsset] is updated to the current day. According to the sponsor the reason for this check is to make sure that a bond that should be expired doesn't get transferred while the epoch hasn't yet been updated.

_transfer function in BondNFT: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L329

    function _transfer(
        address from,
        address to,
        uint256 _id
    ) internal override {
        Bond memory bond = idToBond(_id);
        require(epoch[bond.asset] == block.timestamp/DAY, "Bad epoch");
        require(!bond.expired, "Expired!");
        unchecked {
            require(block.timestamp > bond.mintTime + 300, "Recent update");
            userDebt[from][bond.asset] += bond.pending;
            bondPaid[_id][bond.asset] += bond.pending;
        }
        super._transfer(from, to, _id);
    }

As can be seen above, if epoch[tigAsset] is not set to the same day of the transfer, the transfer will fail and the impacts in the impact section will happen.

Hardhat POC

There is already an implemented test showing that transfers fail when epoch[tigAsset] is not updated: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/test/09.Bonds.js#L472

    it("Bond can only transferred if epoch is updated", async function () {
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("3000"));
      await lock.connect(owner).lock(StableToken.address, ethers.utils.parseEther("3000"), 365);

      await network.provider.send("evm_increaseTime", [864000]);
      await network.provider.send("evm_mine");

      await expect(bond.connect(owner).safeTransferMany(user.address, [1])).to.be.revertedWith("Bad epoch");
    });

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

The reason for the check is to validate that a bond.expired updated according to the actual timestamp. Instead of having

        require(epoch[bond.asset] == block.timestamp/DAY, "Bad epoch");
        require(!bond.expired, "Expired!");

You could replace it with:

 require(bond.expireEpoch  >= block.timestamp/DAY, "Transfer after expired not allowed");

Lines of code

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

Vulnerability details

Impact

Traders will not be able to:

1. Initiate a market order
2. Add margin
3. Add to position
4. initiate limit order

If USDT is set as the margin asset and protocol is deployed on ethereum.

(Note: this issue was submitted after consulting with the sponsor even though currently there are no plans to deploy the platform on ethereum)

Proof of Concept

USDT has a race condition protection mechanism on ethereum chain: It does not allow users to change the allowance without first changing the allowance to 0.

approve function in USDT on ethereum: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L205

    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);
    }

in Trading if users use USDT as margin to:

1. Initiate a market order
2. Add margin
3. Add to position
4. initiate limit order

The transaction will revert.

This is due to the the _handleDeposit which is called in all of the above uses. _handleDeposit calls the USDT margin asset approve function with type(uint).max. From the second time approve will be called, the transaction will revert.

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

    function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault
, ERC20PermitData calldata _permitData, address _trader) internal {
------
            IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
            IERC20(_marginAsset).approve(_stableVault, type(uint).max);
            IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);
------
    }

Tools Used

VS Code

Recommended Mitigation Steps

No need to to approve USDT every time. The protocol could:

1. Keep a record if allowance was already set on an address
2. Create an external function that can be called by the owner to approve the a token address

Lines of code

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

Vulnerability details

Impact

1. Users will not be able to release their BondNFT even after lock period
2. Users will not receive rewards even if funds exists.
3. DOS to any function in lock that interacts with BondNFT

Proof of Concept

The flow of transferring rewards to BondNFT is as follows:

1. Rewards are added to GovNFT to distribute between all holders of the GovNFT.
2. claimGovFees() is called, it loops over every tigAsset of BondNFT.
3. claim(tigAsset) is called on the GovNFT to retrieve rewards to lock.
4. BondNFT.distribute(tigAsset, balanceAdded) is called to distribute rewards to accounting
5. tigAssets are transfered from Lock to BondNFT
6. A loop between the last day of reward distribution to current day is generated to update state variables.
7. Loop continue to #3

These steps contain loops inside loops. Loop of #2 contains 5 external calls #3 has 1 external call #5 has 1 external call #6 has 2*X (time difference between current day and previous distribution) changes of state variables

In total: External calls: <numberOfAllTimeAssets>*7 State variable store: <numberOfWhitelistedAssets> * 2 * X

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

    function claimGovFees() public {
        address[] memory assets = bondNFT.getAssets();

        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);
        }
    }

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

    function distribute(
        address _tigAsset,
        uint _amount
    ) external {
        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;
        IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount);
        unchecked {
            uint aEpoch = block.timestamp / DAY;
            if (aEpoch > epoch[_tigAsset]) {
                for (uint i=epoch[_tigAsset]; i<aEpoch; i++) {
                    epoch[_tigAsset] += 1;
                    accRewardsPerShare[_tigAsset][i+1] = accRewardsPerShare[_tigAsset][i];
                }
            }
            accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];
        }
        emit Distribution(_tigAsset, _amount);
    }

Consider the following scenario: Its been more then a few years since the project launch, multiple tigAssets have been created. Stakers have participated in the protocol by locking their tigAssets in lock and receiving BondNFTs. Suddenly the protocol has low liquidity of tigAssets. It pauses the creation and rewarding of specific assets by blacklisting the tigAssets for distribution for 200 days until the protocol is able to build up more liquidity. After 200 days, it allows the tigAsset bonds to be rewarded and created. Out of gas exception will happen when rewards will be distributed to accounting

Tools Used

VS Code

Recommended Mitigation Steps

1. Split the functions into smaller ones that can be called individually per tigAsset if needed.
2. Do not loop of blacklisted assets

Judge has assessed an item in Issue #97 as M risk. The relevant finding follows:

DOS StableVault deposit and withdraws if ERC20 with more than 18 decimals used. Description withdraw and deposit functions in StableVault will revert if ERC20 token with more than 18 decimals is used. This is due to an revert when trying to subtract the token decimals from hardcoded 18.

deposit: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L49

function deposit(address _token, uint256 _amount) public {
    require(allowed[_token], "Token not listed");
    IERC20(_token).transferFrom(_msgSender(), address(this), _amount);
    IERC20Mintable(stable).mintFor(
        _msgSender(),
        _amount*(10**(18-IERC20Mintable(_token).decimals()))
    );
}

Recommendation: Add a check that token decimals is under 18 in listToken. withdraw: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L67:

function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
    IERC20Mintable(stable).burnFrom(_msgSender(), _amount);
    _output = _amount/10**(18-IERC20Mintable(_token).decimals());
    IERC20(_token).transfer(
        _msgSender(),
        _output
    );
}

---

Lines of code

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

Vulnerability details

Impact

The protocol uses Chainlink to verify price feeds. According to the documentation:

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.

In the current implementation, the API used to check the Chainlink price feed is deprecated and can return stale prices. Therefore, 2% prices difference can happen more frequently as oracle network prices can be ahead of Chainlink latestAnswer().

This is especially true in:

1. Volatile pairs.
2. Events that cause assets to drop/increase rapidly.

Proof of Concept

The function verifyPrice is used to validate the price feed during trading operations. It then validates that the price received from the oracle network is not more or less then 2% from the Chainlink price result.

verifyPrice in TradingLibrary: https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L113

    function verifyPrice(
        uint256 _validSignatureTimer,
        uint256 _asset,
        bool _chainlinkEnabled,
        address _chainlinkFeed,
        PriceData calldata _priceData,
        bytes calldata _signature,
        mapping(address => bool) storage _isNode
    )
        external view
    {
-------
        if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
                require(
                    _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                    _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
                );
            }
        }
    }

As you can see in the above function: IPrice(_chainlinkFeed).latestAnswer(); is used.

latestAnswer is deprecated by Chainlink and should not be used: https://docs.chain.link/data-feeds/price-feeds/api-reference/#latestanswer

latestAnswer can return stale prices, there is no valid way to check the time of the received price. latestRoundData should be used.

Tools Used

VS Code

Recommended Mitigation Steps

Use latestRoundData instead of latestAnswer. When latestRoundData you can validate that the timestamp of the price received is recent and matching to _validSignatureTimer threshold you already check the oracle network price data on.

If price is stale, either revert the transaction or don't check the 2% difference.

Additionally round completeness should also be checked