Tigris Trade contest - HollaDieWaldfee's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L10 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L61-L76 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L84-L92 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L98-L105

Vulnerability details

Impact

The Lock contract (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L10) allows end-users to interact with bonds.

There are two functions that allow to lock some amount of assets. The first function is Lock.lock (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L61-L76) which creates a new bond. The second function is Lock.extendLock (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L84-L92). This function extends the lock for some _period and / or increases the locked amount by some _amount.

The issue is that the Lock.extendLock function does not increase the value in totalLocked[_asset]. This however is necessary because totalLocked[_asset] is reduced when Lock.release (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L98-L105) is called.

Therefore only the amount of assets deposited via Lock.lock can be released again. The amount of assets deposited using Lock.extendLock can never be released again because reducing totalLocked[_asset] will cause a revert due to underflow.

So the amount of assets deposited using Lock.extendLock is lost.

Proof of Concept

1. User A calls Lock.lock to lock a certain _amount (amount1) of _asset for a certain _period.
2. User A calls then Lock.extendLock and increases the locked amount of the bond by some amount2
3. User A waits until the bond has expired
4. User A calls Lock.release. This function calculates totalLocked[asset] -= lockAmount;. Which will cause a revert because the value of totalLocked[asset] is only amount1

You can add the following test to the Bonds test in Bonds.js:

describe("ReleaseUnderflow", function () {
    it("release can cause underflow", async function () {
        await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("110"));
        // Lock 100 for 9 days
        await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 9);

        await bond.connect(owner).setManager(lock.address);

        await stabletoken.connect(user).approve(lock.address, ethers.utils.parseEther("10"));

        // Lock another 10
        await lock.connect(user).extendLock(1, ethers.utils.parseEther("10"), 0);

        await network.provider.send("evm_increaseTime", [864000]); // Skip 10 days
        await network.provider.send("evm_mine");

        // Try to release 110 after bond has expired -> Underflow
        await lock.connect(user).release(1);
    });
});

Run it with npx hardhat test --grep "release can cause underflow".
You can see that it fails because it causes an underflow.

Tools Used

VSCode

Recommended Mitigation Steps

Add totalLocked[_asset] += amount to the Lock.extendLock function.

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.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L255-L305 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L278 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L293-L294

Vulnerability details

Impact

When opening a position, an opening fee has to be paid.
You can see this in the Trading.initiateMarketOrder function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163-L210).

It deposits the whole _tradeInfo.margin (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L180) but the trade is only created with the _marginAfterFees (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L185), i.e. the margin minus the opening fees.

This is how paying the opening fee should work.

There is an issue in the Trading.addToPosition function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L255-L305). Instead of depositing the whole _addMargin which is the margin that is added to the position, the _addMargin - _fee (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L278) is deposited. This is the same amount that is then added to the position (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L293-L294).

In effect this means that no opening fees are deducted from the margin that is deposited.

Therefore increasing a position is free for the trader (except gas fees).

A trader can exploit this by opening a small position first (which must be as big as the minimum position size).

The trader can then increase his position to the desired size using the Trading.addToPosition function. So the trader only needs to pay the opening fee for the minimum position size.

Proof of Concept

1. The trader opens a position with the Trading.initiateMarketOrder function. The position is big enough to meet the minimum position size requirement (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/TradingExtension.sol#L218)
2. The trader calls the Trading.addToPosition function to increase his position to the desired size.
   The amount he needs to deposit into the StableVault is _addMargin - fee:

_handleDeposit(
    _trade.tigAsset,
    _marginAsset,
    _addMargin - _fee,
    _stableVault,
    _permitData,
    _trader
);

The _newMargin is calculated as _trade.margin + _addMargin - fee:

_addMargin -= _fee;
uint _newMargin = _trade.margin + _addMargin;

The margin of the trade is then changed to be the _newMargin:

position.addToPosition(
    _trade.id,
    _newMargin,
    _newPrice
);

Code from Position.addToPosition:

function addToPosition(uint256 _id, uint256 _newMargin, uint256 _newPrice) external onlyMinter {
    _trades[_id].margin = _newMargin;
    _trades[_id].price = _newPrice;
    initId[_id] = accInterestPerOi[_trades[_id].asset][_trades[_id].tigAsset][_trades[_id].direction]*int256(_newMargin*_trades[_id].leverage/1e18)/1e18;
}

In the end the trader deposited _addMargin - fee and the position was also increased by _addMargin - fee. So the trader did not pay fees for increasing his position.

Tools Used

VSCode

Recommended Mitigation Steps

The solution is very simple. The trader must deposit the whole _addMargin into the vault.

         _handleDeposit(
             _trade.tigAsset,
             _marginAsset,
-            _addMargin - _fee,
+            _addMargin,
             _stableVault,
             _permitData,
             _trader
         };

Lines of code

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

Vulnerability details

Impact

When calling Lock.claimGovFees (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L110), assets that are set to be not allowed or assets that don't have any shares yet in the BondNFT contract will cause a silent failure in BondNFT.distribute (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L215).
The funds from the GovNFT contract will get transferred into the Lock contract and then will be stuck there. They cannot be recovered.

Proof of Concept

1. An asset is added to the BondNFT contract by calling BondNFT.addAsset (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L349)
2. There are no bonds yet for this asset so the amount of shares for the asset is zero
3. Lock.claimGovFees (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L110) is called
4. Funds are transferred from the GovNFT contract to the Lock contract (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Lock.sol#L115)
5. The call to BondNFT.distribute now fails quietly without reverting the transaction:

    if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;

6. The funds are now stuck in the Lock contract. They cannot be recovered.

Tools Used

VSCode

Recommended Mitigation Steps

A naive solution would be to use revert instead of return in BondNFT.distribute such that funds are either transferred from GovNFT to Lock and then to BondNFT or not at all.

     ) external {
-        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;
+        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) revert;
         IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount);
         unchecked {
             uint aEpoch = block.timestamp / DAY;

This however is an incomplete fix because if there is a single "bad" asset, rewards for the other assets cannot be distributed either.

Moreover functions like Lock.lock and Lock.release rely on Lock.claimGovFees to not revert.

So you might allow the owner to rescue stuck tokens from the Lock contract. Of course only allow rescuing the balance of the Lock contract minus the totalLocked of the asset in the Lock contract such that the locked amount cannot be rescued.

Lines of code

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

Vulnerability details

Impact

The BondNFT and GovNFT contracts provide a safeTransferMany function:

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

GovNFT.safeTransferMany: https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L245

The issue is that both of these functions are not actually "safe".

"safe" in the context of an ERC721 means that if the receiver of a token transfer is a contract, it must implement the onERC721Received function.

The contract signals by implementing it that it can handle ERC721 transfers.

So in this case it is misleading that the functions are labelled "safe" but are not actually "safe".

Especially since BondNFT.safeTransferFromMany and GovNFT.safeTransferFromMany are indeed "safe" since they call safeTransferFrom internally whereas the safeTransferMany functions call _transfer internally. So I believe this is just an oversight that occurred while coding and the safeTransferMany functions should call a "safe" function internally as well.

Since it looks like the safeTransferMany functions are safe when indeed they are not, tokens can be transferred to a contract that cannot handle ERC721s, thereby the tokens are lost.

Proof of Concept

1. User A owns two BondNFT tokens
2. User A wants to send the tokens to a contract which he assumes can handle ERC721s but is not sure. By calling BondNFT.safeTransferMany, he thinks it is first checked whether the contract implements onERC721Received since "safe" has this meaning for ERC721 transfers.
3. The contract does not implement onERC721Received and cannot handle ERC721s. The tokens are lost.

Tools Used

VSCode

Recommended Mitigation Steps

In BondNFT.safeTransferMany and GovNFT.safeTransferMany, change the line

_transfer(_msgSender(), _to, _ids[i]);

to

safeTransferFrom(_msgSender(), _to, _ids[i]);  

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/PairsContract.sol#L8 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Position.sol#L99 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L314 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163

Vulnerability details

Impact

The PairsContract (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/PairsContract.sol#L8) keeps track of the open interest for each [asset][tigAsset] pair.

The Trading.executeLimitOrder (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480) function causes the open interest to be increased.
However the amount by which it is increased is too big because it does not reduce the margin by the amount of fees that is paid upon opening a position.

Also once the position is closed the amount by which the open interest is reduced DOES respect the opening fee.
So the amount by which the open interest is increased is bigger than the amount by which the open interest is reduced when the trade is closed.

Therefore every trade that is opened by executing a limit order causes the open interest to rise by a small amount.

This in turn causes issues in the calculation of the funding rate which relies on the open interst (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Position.sol#L99).

There is no immediate loss of funds for a single trade. However in the long run the wrong funding rate calculation can make the Exchange unusable.

So I mark this issue as "Medium".

Proof of Concept

A limit order is created by calling the Trading.initiateLimitOrder function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L314).

It mints a trade and saves the full margin in it. The opening fees are only later incorporated. After the open interest is increased.

So once the limit order is created, Trading.executeLimitOrder must be called (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480).

In this function the trade is read into memory:

IPosition.Trade memory trade = position.trades(_id);

Then, without adjusting the trade.margin, the open interest is increased:

if (trade.direction) {
    tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
} else {
    tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
}

The trade margin is only later adjusted:

position.executeLimitOrder(_id, trade.price, trade.margin - _fee);

You can also easily see that this is a vulnerability by comparing the Trading.executeLimitOrder function to e.g. Trading.initiateMarketOrder (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163), which increases the open interest by the _positionSize amount:

if (_tradeInfo.direction) {
    tradingExtension.modifyLongOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
} else {
    tradingExtension.modifyShortOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
}

This amount uses the margin adjusted by fees:

uint256 _positionSize = _marginAfterFees * _tradeInfo.leverage / 1e18;

Tools Used

VSCode

Recommended Mitigation Steps

Fix:

                 trade.price -= trade.price * _spread / DIVISION_CONSTANT;
             }
             if (trade.direction) {
-                tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
+                tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, (trade.margin-_fee)*trade.leverage/1e18);
             } else {
-                tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
+                tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, (trade.margin-_fee)*trade.leverage/1e18);
             }
             _updateFunding(trade.asset, trade.tigAsset);
             position.executeLimitOrder(_id, trade.price, trade.margin - _fee);

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.