Blacklisting a tigAsset results in loss of all user staked funds without earning rewards

[code423n4]

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.

[GalloDaSballo]

Results in loss of reward, but should allow withdrawal,the POC looks incorrect

[TriHaz]

Only results in loss of rewards, and it's not permanent as an asset can be set to allowed again, also setting that is only possible by the owner, so I disagree with severity, should be low risk.

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[GalloDaSballo]

While there's some extra observations, am going to award this as a valid not allowed asset cannot be distributed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #73

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

I recommend the warden to file separate reports for separate issues as otherwise you will get your reports potentially misinterpreted

[c4-judge]

GalloDaSballo marked the issue as satisfactory