Removed BondNFT assets rewards will be lost

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L110-L120
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L275-L280
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L357-L360
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L211-L215

Vulnerability details

Impact

BondNFT has its allowedAsset[] for allowed assets. But from Lock.sol#claimGovFees(), GovNFT will claim any asset rewards no matter it is allowed in BondNFT or not. So some asset not in BondNFT allowedAsset[] could possibly be collected from GovNFT claim(), in this case the corresponding rewards will be lost and locked in the contract.

Proof of Concept

Since the assets could change over time, the admin sometimes need to maintain the assets[] and allowedAsset[]. But for GovNFT, any asset can be claimed as rewards.

File: contracts/Lock.sol
110:     function claimGovFees() public {
111:         address[] memory assets = bondNFT.getAssets();
112: 
113:         for (uint i=0; i < assets.length; i++) {
114:             uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
115:             IGovNFT(govNFT).claim(assets[i]);
116:             uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
117:             IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
118:             bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
119:         }
120:     }

File: contracts/GovNFT.sol
275:     function claim(address _tigAsset) external {
276:         address _msgsender = _msgSender();
277:         uint256 amount = pending(_msgsender, _tigAsset);
278:         userPaid[_msgsender][_tigAsset] += amount;
279:         IERC20(_tigAsset).transfer(_msgsender, amount);
280:     }

In BondNFT, some asset could be set as false. Then the BondNFT.sol#distribute() will silently return and do nothing.

File: contracts/BondNFT.sol
357:     function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
358:         require(assets[assetsIndex[_asset]] == _asset, "Not added");
359:         allowedAsset[_asset] = _bool;
360:     }

211:     function distribute(
212:         address _tigAsset,
213:         uint _amount
214:     ) external {
215:         if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;

In this situation, although some asset might be claimed by GovNFT, and the balanceBefore and balanceAfter were changed, this part of rewards will be ignored in BondNFT.sol#distribute(). And be locked in the contract.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• In Lock.sol#claimGovFees(), Skip the disallowed assets.

File: contracts/Lock.sol
    function claimGovFees() public {
        address[] memory assets = bondNFT.getAssets();

        for (uint i=0; i < assets.length; i++) {
            if (bondNFT.allowedAsset(assets[i])) {
                uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
                IGovNFT(govNFT).claim(assets[i]);
                uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
                IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
                bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
            }
        }
    }

• In GovNFT, also enforce some allowedAssets list, to be synchronized with BondNFT.
• Use some admin function to handle the disallowed assets in GovNFT.

[GalloDaSballo]

Looks off, if no reward is available we will receive zero
If the token is disallowed but some is still available as reward, we should claim it

Will double check

[TriHaz]

Ideally, assets in BondNFT and GovNFT are synced.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #73

[c4-judge]

GalloDaSballo marked the issue as satisfactory