BondNFT.sol: claim function distributes rewards of expired bond across too many shares

[code423n4]

Lines of code

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

Vulnerability details

Impact

Note: I have submitted a different issue (issue 68) 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) distributes rewards that are accumulated for an expired bond across all shares.

The issue is that the rewards are distributed across ALL shares when instead they should be distributed across ALL shares minus the shares of the bond that is expired. This is because clearly the shares of the bond are expired and can no longer earn rewards.

This causes the accRewardsPerShare to be lower than it should be and results in a loss for all shareholders.

Proof of Concept

The vulnerable code is this:

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

In the last line, _pendingDelta is distributed across totalShares[bond.asset]. This however includes bond.shares, i.e. the shares of the expired bond. However since the bond is expired, there are now less shares that are able to earn rewards (totalShares[bond.asset] - bond.shares).

There are two paths that BondNFT.claim can be called.

The first path is Lock.release -> BondNFT.release -> BondNFT.claim. In this case, the totalShares[bond.asset] are adjusted in BondNFT.release before BondNFT.claim is called:

totalShares[bond.asset] -= bond.shares;
(uint256 _claimAmount,) = claim(_id, bond.owner);

So this path is not vulnerable.

However there is another path: Lock.claim -> BondNFT.claim. On this path, totalShares[bond.asset] is not reduced. So this is the path that causes the issue.

Tools Used

VSCode

Recommended Mitigation Steps

Change the code in the BondNFT.claim function like this:

             if (bond.expired) {
                 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];
+                    accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/(totalShares[bond.asset] - bond.shares);
                 }
             }
             bondPaid[_id][bond.asset] += amount;

And move the line totalShares[bond.asset] -= bond.shares; in BondNFT.release below the call to claim:

         amount = bond.amount;
         unchecked {
-            totalShares[bond.asset] -= bond.shares;
             (uint256 _claimAmount,) = claim(_id, bond.owner);
+            totalShares[bond.asset] -= bond.shares;
             amount += _claimAmount;
         }
         asset = bond.asset;

Now both paths work with the correct amount of shares.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #523

[c4-judge]

GalloDaSballo marked the issue as duplicate of #630

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory