Compromised or malicious owner of `GovNFT` contract can call `_bridgeMint` function on Chain A to block a Governance NFT's holder from bridging such NFT from Chain B to Chain A

[code423n4]

Lines of code

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

Vulnerability details

Impact

After a Governance NFT is minted and transferred to a user on Chain A, this user can bridge it to Chain B. Afterwards, because this NFT is already burned on Chain A, the owner of the GovNFT contract, who can possibly become compromised or malicious, can call the following GovNFT._bridgeMint function on Chain A to mint the same NFT on Chain A. Later, when the corresponding NFT's holder calls the GovNFT.crossChain function on Chain B for bridging such NFT from Chain B to Chain A, the GovNFT.lzReceive function on Chain A is called, which eventually calls the GovNFT._bridgeMint function on Chain A. However, calling the GovNFT._bridgeMint function on Chain A for this NFT reverts because the same NFT was minted on Chain A already due to the GovNFT contract's owner's previous GovNFT._bridgeMint function call. As a result, the holder of such Governance NFT loses the ability for bridging such NFT from Chain B to Chain A and is not able to earn rewards from the DAO fees generated by the trades on Chain A.

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

    function _bridgeMint(address to, uint256 tokenId) public {
        require(msg.sender == address(this) || _msgSender() == owner(), "NotBridge");
        require(tokenId <= 10000, "BadID");
        for (uint i=0; i<assetsLength(); i++) {
            userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
        }
        super._mint(to, tokenId);
    }

Proof of Concept

For example, based on this protocol's deployed contract addresses, querying 55 using https://polygonscan.com/address/0x5DF98AA475D8815df7cd4fC4549B5c150e8505Be#readContract#F14 indicates that Governance NFT id 55 does not exist on Polygon but querying 55 using https://arbiscan.io/address/0x303c470c0e0342a1CCDd70b0a17a14b599FF1474#readContract#F18 indicates that Governance NFT id 55 exists on Arbitrum.

The following test can then be added in the Crosschain related functions describe block in test\05.GovNFT.js. This test will pass to demonstrate the described scenario. Please see the comments in this test for more details.

    it.only("Owner of GovNFT contract can call _bridgeMint function on Chain A to block a Governance NFT's holder from bridging such NFT from Chain B to Chain A", async function () {
      // Governance NFT id 55 is owned by a user on Arbitrum.
      // Since Governance NFT id 55 is not owned by anyone on Polygon,
      //   owner of GovNFT contract is able to call _bridgeMint function to mint Governance NFT id 55 on Polygon.
      await govnft.connect(owner)._bridgeMint(user.address, 55);

      // When Governance NFT id 55's holder calls crossChain function on Arbitrum for bridging Governance NFT id 55 from Arbitrum to Polygon,
      //   lzReceive function on Polygon is called, which eventually calls _bridgeMint function on Polygon as well.
      // However, calling _bridgeMint function for Governance NFT id 55 reverts
      //   because it was minted on Polygon already due to GovNFT contract's owner's _bridgeMint function call.
      // Hence, Governance NFT id 55's holder fails to bridge Governance NFT id 55 from Arbitrum to Polygon.
      await expect(govnft.connect(owner)._bridgeMint(user.address, 55)).to.be.revertedWith("ERC721: token already minted");
    });

Tools Used

VSCode

Recommended Mitigation Steps

The _msgSender() == owner() require condition can be removed from https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L65 to prevent the owner of the GovNFT contract from arbitrarily minting a Governance NFT.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Primary because POC

[TriHaz]

We are aware of the centralization risks, initially, all contracts will have a multi-sig as owner to prevent a sole owner, later on a DAO could be the owner.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[GalloDaSballo]

Making dup of #377 as it highlights a centralization risk

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory