BondNFT.sol#claim() uses the wrong denominator

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L180

Vulnerability details

Impact

In BondNFT.sol#claim(), accRewardsPerShare[][] is amended to reflect the expired shares. But the current formula used is wrong

• accRewardsPerShare might be inaccurate.
• some users could lose reward due to wrong accRewardsPerShare, some users might receive undeserved rewards.

Proof of Concept

The rationale behind the unchecked block below seems to take into account the shares of reward of the expired bond. However, the denominator is wrong.

File: contracts/BondNFT.sol
168:     function claim(
169:         uint _id,
170:         address _claimer
171:     ) public onlyManager() returns(uint amount, address tigAsset) {
    
177:             if (bond.expired) {
178:                 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]);
179:                 if (totalShares[bond.asset] > 0) {
180:                     accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
181:                 }
182:             }
183:             bondPaid[_id][bond.asset] += amount;

Assuming, on day 0, epoch[tigAsset] is 100, accRewardsPerShare[tigAsset][100] is 500. totalShares[tigAsset] is 100. (decimal calculation will be omitted for simplicity)

Some user will do the following:

• on day 1, some user mints 1 bondNFT, with shares of 900, expires 7 days later, now totalShares[tigAsset] is 1000.
• on day 7, distribute(tigAsset, 100*1000), accRewardsPerShare[tigAsset][107] is 600.
• on day 9, accRewardsPerShare[tigAsset][109] is 650, due to new distribute(tigAsset, 50*1000).
• on day 9, the user calls Lock.sol#claim(id).
  _pendingDelta will be 900 * 650 - 900 * 600 = 45000.
  According to line 180, accRewardsPerShare[tigAsset][109] will be increased by 45000 / 1000 = 45, change from 650 to 695.

Since the 900 shares is expired, this portion should not be counted in the accRewardsPerShare[][] reward update. So the intended accRewardsPerShare[][] increase should be 50*1000 / 100 = 500, accRewardsPerShare[tigAsset][109] should be 600 + 500 = 1100 instead of 695.

The reason is, in line 180, the denominator still includes the expired shares, which should be excluded.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Change the denominator:

180:                     accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/(totalShares[bond.asset] - bond.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