Unreleased locks cause the reward distribution to be flawed in BondNFT

[code423n4]

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.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Making primary because this offers a test for the sponsor

[TriHaz]

Since this causes a loss of funds for every bond holder I rate it as HIGH.

Funds are not lost, they will be redistributed when the bond is expired. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L180

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

Also see #523

[GalloDaSballo]

I've asked the Warden for additional proof:

const { expect } = require("chai");
const { deployments, ethers } = require("hardhat");

async function getTimestamp() {
  const blockNumBefore = await ethers.provider.getBlockNumber();
  const blockBefore = await ethers.provider.getBlock(blockNumBefore);
  const timestampBefore = blockBefore.timestamp;
  return timestampBefore;
}

async function getAEpoch() {
  return parseInt(await getTimestamp()/86400);
}

describe("Bonds", function () {

  let owner;
  let treasury;
  let user;
  let rndAddress;

  let Lock;
  let lock;

  let Bond;
  let bond;

  let GovNFT;
  let govnft;

  let StableToken;
  let stabletoken;

  beforeEach(async function () {
    await deployments.fixture(['test']);
    [owner, treasury, user, rndAddress, user2] = await ethers.getSigners();
    Lock = await deployments.get("Lock");
    lock = await ethers.getContractAt("Lock", Lock.address);
    Bond = await deployments.get("BondNFT");
    bond = await ethers.getContractAt("BondNFT", Bond.address);
    StableToken = await deployments.get("StableToken");
    stabletoken = await ethers.getContractAt("StableToken", StableToken.address);
    GovNFT = await deployments.get("GovNFT");
    govnft = await ethers.getContractAt("GovNFT", GovNFT.address);
    await stabletoken.connect(owner).setMinter(owner.address, true);
    await govnft.connect(owner).mintMany(1);
    await stabletoken.connect(user).approve(Lock.address, ethers.utils.parseEther("3000"));
    await stabletoken.connect(user2).approve(Lock.address, ethers.utils.parseEther("3000"));
    await stabletoken.connect(owner).approve(Lock.address, ethers.utils.parseEther("100000000000"));
    await stabletoken.connect(owner).approve(Bond.address, ethers.utils.parseEther("100000000000"));
    await stabletoken.connect(owner).approve(GovNFT.address, ethers.utils.parseEther("100000000000"));
    await govnft.connect(owner).safeTransferMany(Lock.address, [1]);
    await lock.editAsset(StableToken.address, true);
    await bond.addAsset(StableToken.address);
  });
  describe("Ruhum", () => {
  it("BondRuhum", async function() {
    // both users get stabletokens that they will lock up
    await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("1000"));
    await stabletoken.connect(owner).mintFor(user2.address, ethers.utils.parseEther("1000"));

    // user 1 locks them for 100 days
    await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 100);
    // user 2 for 10 days
    await lock.connect(user2).lock(StableToken.address, ethers.utils.parseEther("1000"), 10);

    // awards are distributed to all the users who have a lock
    await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
    await bond.connect(owner).distribute(stabletoken.address, ethers.utils.parseEther("1000"));

    // user 1 locked less tokens for longer
    // while user 2 locked more tokens for a shorter amount of time.
    // They both receive the same amount of awards because of that.
    [,,,,,,,pending,,,] = await bond.idToBond(1);
    expect(pending).to.be.equals("499999999999999999986");
    [,,,,,,,pending,,,] = await bond.idToBond(2);
    expect(pending).to.be.equals("499999999999999999986");

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

    // user 2's lock has expired. They shouldn't earn any rewards for that anymore.

    // we distribute another batch of rewards
    await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
    await bond.distribute(stabletoken.address, ethers.utils.parseEther("1000"));

    // user 2 doesn't receive any more awards. Their lock has expired but they have not released it yet.
    // Thus, their pending amount is equal to the one before the second award distribution
    [,,,,,,,pending,,,] = await bond.idToBond(2);
    expect(pending).to.be.equals("499999999999999999986");

    // since user 1 is the only remaining holder of a valid lock, we'd expect them to receive all the shares.
    [,,,,,,,pending,,,] = await bond.idToBond(1);
    // But they don't. They only receive half of the second reward's batch. 
    // expect(pending).to.be.equals("1499999999999999999960");

    expect(await stabletoken.balanceOf(user.address)).to.be.equal("900000000000000000000");
    expect(await stabletoken.balanceOf(user2.address)).to.be.equal("0");

    await lock.connect(user2).claim(2);

    expect(await stabletoken.balanceOf(user2.address)).to.be.equals("499999999999999999986");

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

    await lock.connect(user).claim(1);
    // 900000000000000000000 + 1499999999999999999960
    // 2399999999999999999960
    // AssertionError: Expected "2149999999999999999966" to be equal 2399999999999999999960
    expect(await stabletoken.balanceOf(user.address)).to.be.equals("2399999999999999999960");

  });

  it("BondRuhum2", async function() {
    // both users get stabletokens that they will lock up
    await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("1000"));
    await stabletoken.connect(owner).mintFor(user2.address, ethers.utils.parseEther("1000"));

    // user 1 locks them for 100 days
    await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 100);
    // user 2 for 10 days
    await lock.connect(user2).lock(StableToken.address, ethers.utils.parseEther("1000"), 10);

    // awards are distributed to all the users who have a lock
    await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
    await bond.connect(owner).distribute(stabletoken.address, ethers.utils.parseEther("1000"));

    // user 1 locked less tokens for longer
    // while user 2 locked more tokens for a shorter amount of time.
    // They both receive the same amount of awards because of that.
    [,,,,,,,pending,,,] = await bond.idToBond(1);
    expect(pending).to.be.equals("499999999999999999986");
    [,,,,,,,pending,,,] = await bond.idToBond(2);
    expect(pending).to.be.equals("499999999999999999986");
    await network.provider.send("evm_increaseTime", [86400 * 11]); // Skip 11 days
    await network.provider.send("evm_mine");
    // user 2's lock has expired. They release their lock
    await lock.connect(user2).release(2);

    // we distribute another batch of rewards
    await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
    await bond.distribute(stabletoken.address, ethers.utils.parseEther("1000"));

    // since user 1 is the only remaining holder of a valid lock, we'd expect them to receive all the shares.
    [,,,,,,,pending,,,] = await bond.idToBond(1);
    // And they do because user 2 has released their lock prior to the second batch of rewards
    expect(pending).to.be.equals("1499999999999999999960");

    expect(await stabletoken.balanceOf(user.address)).to.be.equal("900000000000000000000");
    expect(await stabletoken.balanceOf(user2.address)).to.be.equal("1499999999999999999986");

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

    await lock.connect(user).claim(1);
    // 900000000000000000000 + 1499999999999999999960
    // = 2399999999999999999960
    expect(await stabletoken.balanceOf(user.address)).to.be.equals("2399999999999999999960");
   });
  })
});

And believe that the finding is valid

I have adapted the test to also claim after, and believe that the lost rewards cannot be received back (see POC and different values we get back)

[GalloDaSballo]

I have to agree with the Wardens warning, however, the release function is public, meaning anybody can break expired locks.

For this reason, I believe that Medium Severity is more appropriate

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GainsGoblin]

@GalloDaSballo This issue seems to be a duplicate of #392