Tigris Trade contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

In the contract GovNFT, it is possible to bridge the governance NFT to other chains. It is also stated in the document that:

NFT holders only earn the profits generated by the platform on the chain that the NFT is on.

It is assumed that there is only one unique NFT per token id. But there is a scenario that can lead to have more than one NFT with the same token id on different chains.

Proof of Concept

• Suppose Bob (honest user who owns an NFT with token id X on chain B) plans to bridge this NFT from chain B to chain A. So, Bob calls the function crossChain to bridge the NFT from chain B to chain A. Thus, his NFT will be burnt on chain B, and it is supposed to be minted on chain A. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L124
• The endpoint is responsible to complete the bridging task on chain A.
• Suppose the endpoint calls the function lzReceive with low gas on chain A, so that the transaction will be not successful.

function lzReceive(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) external override {
        require(_msgSender() == address(endpoint), "!Endpoint");
        (bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft()*4/5, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));
        // try-catch all errors/exceptions
        if (!success) {
            failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
            emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, reason);
        }
    }

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

• Since the transaction was not successful, the message will be added as a failed message.

failedMessages[chainB][Bob's address][_nonce] = keccak256(_payload);

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

• Then, due to network lag (or any server issue, or any failure in endpoint), the endpoint assumes that the transaction is not sent, and it again calls this function with enough gas, so, the NFT with token id X will be minted to Bob's address on chain A. The flow is as follows: lzReceive ==> nonblockingLzReceive ==> _nonblockingLzReceive ==> _bridgeMint
• Now Bob has the NFT on chain A. Moreover, he has a failed message on chain A.
• Then Bob calls the function crossChain to bridge that NFT from chain A to chain B. So, this NFT will be burnt on chain A, and minted to Bob's address on chain B.
• Now, Bob has the NFT with token id X on chain B. Moreover, he has a failed message on chain A.
• He calls the function retryMessage to retry the failed message on chain A. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L206
• By doing so the NFT with token id X will be minted to Bob on chain A. The flow is as follows: retryMessage ==> _nonblockingLzReceive ==> _bridgeMint
• Now Bob has the NFT with token id X on both chain A and chain B. This is the vulnerability.
• Now he can for example sell the NFT on chain B while he is earning the profits generated by the platform on the chain A that the NFT is on.
• Please note that Bob can not call the function retryMessage while he owns the NFT on chain A. Because during minting the NFT, it checks whether the token id exists or not. That is why Bob first bridges the NFT to another chain, and then retries the failed message.

The vulnerability is that when the message is failed, it is not considered as consumed, so in case of a failure in endpoint it is possible to both having failed message and being able to mint it at the same time.

Please note that if this scenario happens again, more NFT with the same token id X will be minted to Bob on different chains.

Tools Used

Recommended Mitigation Steps

It is recommended to track the consumed messages, and add a consumed flag whenever the function lzReceive is called, because it will either immediately mint the NFT or add it to he failed messages to be minted later.

mapping(uint16 => mapping(bytes => mapping(uint64 => bool))) public consumedMessage;

    function lzReceive(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) external override {
        
        require(!consumedMessage[_srcChainId][_srcAddress][_nonce], "already consumed");
        consumedMessage[_srcChainId][_srcAddress][_nonce] = true;

        require(_msgSender() == address(endpoint), "!Endpoint");
        (bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft()*4/5, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));
        // try-catch all errors/exceptions
        if (!success) {
            failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
            emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, reason);
        }
    }

Lines of code

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

Vulnerability details

Impact

Malicious/compromised owner can lead to limitless minting of gov NFT.

Proof of Concept

A malicious or compromised governor can set the endpoint address to any malicious address.

function setEndpoint(ILayerZeroEndpoint _endpoint) external onlyOwner {
        require(address(_endpoint) != address(0), "ZeroAddress");
        endpoint = _endpoint;
    }

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

The malicious endpoint can mint NFT to his own address without any limitation by calling the function lzReceive.

function lzReceive(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) external override {
        require(_msgSender() == address(endpoint), "!Endpoint");
        (bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft()*4/5, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));
        // try-catch all errors/exceptions
        if (!success) {
            failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
            emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, reason);
        }
    }

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

Tools Used

Recommended Mitigation Steps

• There should be a pausing mechanism so that in case of a leaked governor, another security roles are able to pause the malicious minting.
• There should be a delay between changing the address of the endpoint. Because, it is almost impossible to notice the malicious endpoint address assignment with the immediate address change. While, having a delay between setting the pending endpoint and assigning it, gives the protocol or users enough time to investigate the validity of the new endpoint address.

Judge has assessed an item in Issue #113 as M risk. The relevant finding follows:

During handling the open fees, the _tigAsset is distributed to gov. But, it is not approved before to be consumed by gov. So, the first user's transaction to initiate a market order, will fail. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L749

During handling the close fees, the approve max is applied every time before distributing to gov. Actually, this is redundant to approve max every time. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L807

So, the following check is better to be added before distributing to gov in both functions _handleOpenFees and _handleCloseFees:

if(IStable(_tigAsset).allowance(address(this), address(gov)) < _daoFeesPaid){ IStable(_tigAsset).approve(address(gov), type(uint).max); }