Did Not Approve To Zero First

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L110-L120

Vulnerability details

Impact

Inside the Lock contract claimGovFees function can be DOSed on tokens like USDT, KNC because token like this needs to first be approved to zero.

In case claimGovFees reverts because a token like this. The Lock contract functionality is compromised because claimGovFees is internally the first call in all the main contract methods:
release
claim
claimDebt
lock

Notice:
The problem is that a token like this cannot be whitelisted with Lock::editAsset because it comes from bondNFT state variable. And this BondNFT contract can add an unsupported token later with BondNFT::addAsset. And once is added it cannot be removed. Even setAllowedAsset will not stop the DOSing because BondNFT::getAssets() will still return the offending token.

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L110-L120

Tools Used

Manual review

Recommended Mitigation Steps

Approve ERC20 token to zero first.

    function claimGovFees() public {
        address[] memory assets = bondNFT.getAssets();

        for (uint i=0; i < assets.length; 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), 0);           
            IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);
            bondNFT.distribute(assets[i], balanceAfter - balanceBefore);
        }
    }

[c4-judge]

GalloDaSballo marked the issue as duplicate of #104

[c4-judge]

GalloDaSballo marked the issue as satisfactory