Tigris Trade contest - imare's results

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);
        }
    }

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L64-L71 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFTBridged.sol#L48-L55

Vulnerability details

Impact

The method _bridgeMint is used inside NFT token contracts in two cases:

1. When new tokens are to be minted by receiving the message from layerZero. In this case _bridgeMint is called inside _nonblockingLzReceive internal function

2. If a transaction from layerZero fails it can be replayed with the method retryMessage which again calls _nonblockingLzReceive which calls _bridgeMint

As we can see its used to create new tokens from other chains in a first attempt or a latter retry. And because of this it can be only called by the owner (for replaying failed messages) or by the bridge address (when receiving a message).

    require(msg.sender == address(this) || _msgSender() == owner(), "NotBridge");

The main problem is that it can be also called at will by the contract owner because it is a public contract method. And the only check for minting new token is that we pass a tokenId that is smaller then the maximal tokens cap.

It should never happen but by the current contract implementation it can happen. (example by a leakage of the owner private key etc.)

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L64-L71

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFTBridged.sol#L48-L55

Tools Used

Manual review

Recommended Mitigation Steps

Change _bridgeMint access from public to internal. It will still work for replaying and receiving new messages. But it can not be called at will.