Tigris Trade contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L73 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L84-L92 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L103

Vulnerability details

Impact

An underflow in Lock.release() can prevent a lock from being released. That means funds are locked up.

This issue directly affects user funds and doesn't depend on any external requirements. Someone just has to extend their lock and add some funds to it for this be an issue somewhere down the road.

Proof of Concept

When an existing lock is extended, the new amount of assets added to the lock is not included in the totalLocked state variable. When the user tries to release their lock, lockAmount is bigger than totalLocked[asset] causing an underflow:

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

        claimGovFees();

        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        totalLocked[_asset] += _amount;
        
        bondNFT.createLock( _asset, _amount, _period, msg.sender);
    }

    /**
     * @notice Reset the lock time and extend the period and/or token amount
     * @param _id Bond id being extended
     * @param _amount tigAsset amount being added
     * @param _period number of days being added
     */
    function extendLock(
        uint _id,
        uint _amount,
        uint _period
    ) public {
        address _asset = claim(_id);
        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        bondNFT.extendLock(_id, _asset, _amount, _period, msg.sender);
    }

    /**
     * @notice Release the bond once it's expired
     * @param _id Bond id being released
     */
    function release(
        uint _id
    ) public {
        claimGovFees();
        (uint amount, uint lockAmount, address asset, address _owner) = bondNFT.release(_id, msg.sender);
        totalLocked[asset] -= lockAmount;
        IERC20(asset).transfer(_owner, amount);
    }

Here's a test showcasing the issue:

// 09.Bond.js

    it.only("causes an underflow", async function() {
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
      await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 10);
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
      await lock.connect(user).extendLock(1, ethers.utils.parseEther("100"), 0);

      await network.provider.send("evm_increaseTime", [432000 * 3]); // Skip 5 days
      await network.provider.send("evm_mine");

      await lock.connect(user).release(1);
    });

In most cases, this will only affect the very last user to release their lock. But, a whale who holds a majority of the tokens could encounter the same issue as well. It depends on the way to lock was set up.

To unlock these funds, someone else has to create a lock to increase the totalLocked value. There's no way to get all the tokens out. Some amount will always be lost.

Tools Used

none

Recommended Mitigation Steps

In extendLock() add the new locked funds to totalLocked.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L44-L72 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L78-L96

Vulnerability details

Impact

The StableVault + StableToken contracts are essentially a stablecoin built on top of stablecoins. A user deposits 1 stablecoin (USDC, USDT, DAI, etc.) and receives 1 tigUSD in return. At any time, the user can burn their 1 tigUSD to get back 1 stablecoin of their choosing (this is important). There are no checks for the actual price of any of the tokens. The vault assumes that all coins are worth an equal amount, i.e. neither loses its peg. As recent events have shown, that is a bold assumption. Stablecoins have blown up in the past and will probably also blow up in the future. The StableVault contract provides an existential risk to the protocol in return for more concentrated liquidity. Here's a quote from the docs:

The StableVault's purpose is to allow for many tokens of the same kind (such as USDC/DAI/USDT) to be used to trade on Tigris without splitting up the liquidity into multiple individual pools.

Let's go over a basic scenario. StableVault holds assets worth 30M USD (33% DAI, 33% USDT, and 33% USDC), thus there's 30M USD worth of tigUSD in circulation. All the assets are worth the same so there's no problem. Now let's say one of the underlying stablecoins depegs, e.g. USDT. It loses 20% of its value and drops to 80 cents. While the StableVault contract still has the same amount of tokens, the real value dropped to $28M. As a consequence, tigUSD also loses its peg. That will cause people to burn their tigUSD to withdraw the underlying assets of the StableVault. But, they will primarily withdraw the ones that didn't lose their peg (DAI & USDC in our example) since they are worth more. The contract only holds 20M of them tho. It boils down to first come first serve. Anybody whose fast enough will be able to escape the crash by withdrawing coins that kept their peg while everybody else is screwed. You might say, that this is not an issue of the Tigris protocol but of the stablecoin issuers. But, by forcing people to use tigUSD, the protocol is exposing the users to risks that they previously might not have been exposed to. Someone who didn't hold any USDT because they were put off by the potential risk of a blowup will be exposed to it when they use the Tigris protocol. And they might not even know about it.

The StableVault contract in its current form is essentially a stablecoin pool where you can trade stablecoins without any fees: deposit DAI & receive tigUSD -> burn tigUSD and withdraw USDC. A stablecoin swap without any fees. In case of a blowup, the vault could also be abused by people who never used the Tigris protocol to swap between stablecoins at 0 fees.

Stuff like this tends to be brushed off because it's not a direct smart contract vulnerability but an issue with the protocol itself. I strongly believe that the risk taken here is not worth the benefit of having a single USD pool. I also believe that many users won't realize that they are exposed to this risk without clearly reading the docs/contracts.

Proof of Concept

Anybody can deposit one of the whitelisted tokens to receive the same amount of tigUSD. They can also trade in their tigUSD for a stablecoin of their choosing:

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

    /**
    * @notice swap tigAsset to _token
    * @param _token address of the token to receive
    * @param _amount amount of _token
    */
    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
        );
    }

The docs currently say that a vault will only hold one stablecoin. But, the contract was built to support multiple tokens:

    function listToken(address _token) external onlyOwner {
        require(!allowed[_token], "Already added");
        tokenIndex[_token] = tokens.length;
        tokens.push(_token);
        allowed[_token] = true;
    }

    /**
    * @notice stop a token from being allowed in vault
    * @param _token address of the token
    */
    function delistToken(address _token) external onlyOwner {
        require(allowed[_token], "Not added");
        tokenIndex[tokens[tokens.length-1]] = tokenIndex[_token];
        tokens[tokenIndex[_token]] = tokens[tokens.length-1];
        delete tokenIndex[_token];
        tokens.pop();
        allowed[_token] = false;
    }

Having only one stablecoin doesn't make sense anyways. You could just use the underlying asset for trades instead of minting tigUSD and using that. There would be no difference. Thus, it's expected that a vault will hold multiple stablecoins.

Tools Used

none

Recommended Mitigation Steps

StableVault should only allow you to trade tigUSD for the tokens you originally deposited. If you deposited USDC and USDT blows up, you'll be unaffected.

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

03: Lock.claimGovFees() will revert with tokens that have approve race condition protection Some tokens only allow you to approve a new value if the current allowance is set to zero, e.g. USDT.

This is not an issue right now on Polygon or Arbitrum. But, stablecoin contracts on those networks are upgradable and migth change. Also, if you decide to deploy the protocol to other networks, you might encounter this problem.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/deploy/main/01.StableToken.js#L6 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableToken.sol#L38 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableToken.sol#L24 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65

Vulnerability details

Impact

The owner of the StableToken contract can mint new tokens by assigning themselves the minter role. This will result in the tigUSD being depegged because there will be more tokens in circulation than there are assets for in the StableVault contract. It also provides the owner of the project an easy way to steal all the user funds. tigUSD can be exchanged for any underlying stablecoin in StableVault. By minting tigUSD the owner can exchange worthless tokens for actual assets that originally belonged to users, see StableVault docs.

While there are a couple of other centralization risks within this project this one is the most problematic:

1. Owner can steal all user funds in one go leaving affected users with worthless tigUSD tokens.
2. According to the deployment script, the owner of the StableToken contract is the deployer (likely a hot wallet on one of the dev's machines). There's a real risk of these keys being compromised at some point in the future.

Because of that, I rate it as MED instead of QA like the rest of the centralization issues.

Proof of Concept

Assuming there are stablecoins (DAI, USDT, USDC, etc.) worth 1M USD in StableVault, the owner submits a private bundle with the following txs:

1. Owner grants themselves the minter role: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableToken.sol#L38
2. Owner mints themselves 1M tigUSD: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableToken.sol#L24
3. Owner exchanges their tigUSD for assets inside the StableVault contract: https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65-L72

This leaves users with tigUSD tokens that are worth nothing (underlying assets are gone) and the owner with all the user funds.

Tools Used

none

Recommended Mitigation Steps

StableToken should only be able to grant a single address (StableVault) the minter role on deployment.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L150 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L225

Vulnerability details

Impact

After a lock has expired, it doesn't get any rewards distributed to it. But, unreleased locks cause other existing bonds to not receive the full amount of tokens either. The issue is that as long as the bond is not released, the totalShares value isn't updated. Everybody receives a smaller cut of the distribution. Thus, bond owners receive less rewards than they should.

A bond can be released after it expired by the owner of it. If the owner doesn't release it for 7 days, anybody else can release it as well. As long as the owner doesn't release it, the issue will be in effect for at least 7 epochs.

Since this causes a loss of funds for every bond holder I rate it as HIGH. It's likely to be an issue since you can't guarantee that bonds will be released the day they expire.

Proof of Concept

Here's a test showcasing the issue:

// 09.Bonds.js

    it.only("test", async function () {
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("100"));
      await lock.connect(owner).lock(StableToken.address, ethers.utils.parseEther("100"), 100);
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("1000"));
      await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("1000"), 10);
      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
      await bond.distribute(stabletoken.address, ethers.utils.parseEther("1000"));

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

      [,,,,,,,pending,,,] = await bond.idToBond(1);
      expect(pending).to.be.equals("499999999999999999986");
      [,,,,,,,pending,,,] = await bond.idToBond(2);
      expect(pending).to.be.equals("499999999999999999986");


      await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
      await bond.distribute(stabletoken.address, ethers.utils.parseEther("1000"));

      await network.provider.send("evm_increaseTime", [86400 * 3]); // Skip 3 days
      await network.provider.send("evm_mine");

      // Bond 2 expired, so it doesn't receive any of the new tokens that were distributed
      [,,,,,,,pending,,,] = await bond.idToBond(2);
      expect(pending).to.be.equals("499999999999999999986");

      // Thus, Bond 1 should get all the tokens, increasing its pending value to 1499999999999999999960
      // But, because bond 2 wasn't released (`totalShares` wasn't updated), bond 1 receives less tokens than it should.
      // Thus, the following check below fails
      [,,,,,,,pending,,,] = await bond.idToBond(1);
      expect(pending).to.be.equals("1499999999999999999960");

      await lock.connect(user).release(2);

      expect(await stabletoken.balanceOf(user.address)).to.be.equals("1499999999999999999986");

    });

The totalShares value is only updated after a lock is released:

    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");
        if (_releaser != bond.owner) {
            unchecked {
                require(bond.expireEpoch + 7 < epoch[bond.asset], "Bond owner priority");
            }
        }
        amount = bond.amount;
        unchecked {
            totalShares[bond.asset] -= bond.shares;
        // ... 

Tools Used

none

Recommended Mitigation Steps

Only shares belonging to an active bond should be used for the distribution logic.