The function BondNFT.sol#claim() is calculated incorrectly on rewards.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Some users will lose some rewards, and malicious users can use this to gain more rewards.

Proof of Concept

In function BondNFT.sol#claim(), if the claiming bond is expired, the excess of its rewards is re-distributed to all other users:

function claim(
    uint _id,
    address _claimer
) public onlyManager() returns(uint amount, address tigAsset) {
    Bond memory bond = idToBond(_id);
    require(_claimer == bond.owner, "!owner");
    amount = bond.pending;
    tigAsset = bond.asset;
    unchecked {
        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];
            }
        }
        bondPaid[_id][bond.asset] += amount;
    }
    IERC20(tigAsset).transfer(manager, amount);
    emit ClaimFees(tigAsset, amount, _claimer, _id);
}

The following table illustrates the states and transitions of an BondNFT:

• s for shares
• r for reward tokens
• u for unclaimed reward tokens (the pending)
• c for claimed reward tokens

BH	Action or Info	Bond b1	Bond b2	Bond b3	Bond b4
h0	init	100s, 0u, 0c	100s, 0u, 0c	100s, 0u, 0c	0s, 0u, 0c
h1	distribute(60r)	100s, 20u, 0c	100s, 20u, 0c	100s, 20u, 0c	0s, 0u, 0c
h2	b1 expired
h3	distribute(60r)	100s, 40u, 0c	100s, 40u, 0c	100s, 40u, 0c	0s, 0u, 0c
h4	b2 expired
h5	release(b2)	100s, 40u, 0c	100s, 0u, 40c	100s, 40u, 0c	0s, 0u, 0c
h6	create(b4, 100s)	100s, 40u, 0c	0s, 0u, 40c	100s, 40u, 0c	100s, 0u, 0c
h7	claim(b1)	100s, 0u, 20c	0s, 0u, 40c	100s, 46.7u, 0c	100s, 6.7u, 0c

Let's use the above table as an example to analyze the problems.

1. Miscalculation of accRewardsPerShare

The _pendingDelta is the rewards to be re-distributed, and the formula is:

accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];

This formula is wrong. Because the totalShares[bond.asset] includes the expired shares of the claiming bond, which should not be included.

For example, in h7 of the above table claim(b1) is called:

• the _pendingDelta = 20u is re-distributed
• the totalShares = 300s (each of b1, b3, b4 has 100s)

Thus, accRewardsPerShare will increase by _pendingDelta/totalShares = 20u/300s = 0.06666667, which results in the pending reward of b3 and b4 increasing 6.7u each.
This is not correct, the 20u/300s should be corrected to 20u/(300s-100s), subtracting shares of b1 from total shares.

i.e. The h7 should be:

BH	Action or Info	Bond b1	Bond b2	Bond b3	Bond b4
h7	claim(b1)	100s, 0u, 20c	0s, 0u, 40c	100s, 50u, 0c	100s, 10u, 0c

2. Unfair re-distribution

The _pendingDelta is re-distributed to all shares of the current epoch.
This is not fair for bonds that have been released after the expire time of this claiming bond.

For example, in h7 of the above table claim(b1) is called. The _pendingDelta = 20u is re-distributed to b3 and b4.
But in fact it should belong to b2 and b3. Because this _pendingDelta is generated at h3, when b2's share is still there and has not expired, and b4 has not been created yet.

Tools Used

Manual

Recommended Mitigation Steps

The wrong calculation at BondNFT.sol#claim() should be corrected as:

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

And for the "unfair re-distributed", there seems to be no easy way to fix it.
Maybe a scheme like liquidity mining can be considered.

[GalloDaSballo]

Making primary as it has well developed math

[c4-judge]

GalloDaSballo marked the issue as primary issue

[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