Incorrect `isApprovedForAll` check in the `NukeFund.nuke()` function.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/NukeFund/NukeFund.sol#L153-L182

Vulnerability details

Impact

Approved users can't nuke the owner's NFT.

Proof of Concept

In the NukeFund.nuke function, it checks whether nftContract.isApprovedForAll(msg.sender, address(this)) is true from L160 and tries to burn the NFT token of tokenID from L176.

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/NukeFund/NukeFund.sol#L153-L182

        function nuke(uint256 tokenId) public whenNotPaused nonReentrant {
          require(
            nftContract.isApprovedOrOwner(msg.sender, tokenId),
            'ERC721: caller is not token owner or approved'
          );
          require(
            nftContract.getApproved(tokenId) == address(this) ||
L160:       nftContract.isApprovedForAll(msg.sender, address(this)),
            'Contract must be approved to transfer the NFT.'
          );
          [...]
L176:     nftContract.burn(tokenId); // Burn the token
          [...]
        }

In the TraitForgeNft.burn function, it checks whether isApprovedOrOwner(msg.sender, tokenId) is true from L143.

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L141-L151

        function burn(uint256 tokenId) external whenNotPaused nonReentrant {
          require(
L143:       isApprovedOrOwner(msg.sender, tokenId),
            'ERC721: caller is not token owner or approved'
          );
          [...]
        }

msg.sender at L143 is NukeFund contract. In the ERC721._isApprovedOrOwner, it checks as following:

        function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
            address owner = ERC721.ownerOf(tokenId);
            return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
        }

Totally, nuking tokens requires nftContract.isApprovedOrOwner(msg.sender, tokenId) && (nftContract.getApproved(tokenId) == address(this) ||nftContract.isApprovedForAll(msg.sender, address(this))) where this is NukeFund contract. And burning tokens requires isApprovedOrOwner(msg.sender, tokenId) where msg.sender is NukeFund contract. Checking isApprovedOrOwner(msg.sender, tokenId) is same as msg.sender == owner || getApproved(tokenId) == msg.sender || isApprovedForAll(owner, msg.sender). So, nuke function should check nftContract.isApprovedForAll(nftContract.ownerOf(tokenId), address(this)) instead of nftContract.isApprovedForAll(msg.sender, address(this))

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to change the code as following:

    function nuke(uint256 tokenId) public whenNotPaused nonReentrant {
      require(
        nftContract.isApprovedOrOwner(msg.sender, tokenId),
        'ERC721: caller is not token owner or approved'
      );
      require(
        nftContract.getApproved(tokenId) == address(this) ||
-         nftContract.isApprovedForAll(msg.sender, address(this)),
+         nftContract.isApprovedForAll(nftContract.ownerOf(tokenId), address(this)),
        'Contract must be approved to transfer the NFT.'
      );
      [...]
    }

Assessed type

Access Control

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Insufficient quality

[KupiaSecAdmin]

This report clearly demonstrates the presence of a bug.

Scenario

Consider the following scenario:

1. Alice approved the NukeFund contract as an operator.(Alice called setApprovalForAll(nukeFund, true).)
2. Alice is the owner of an entity.
3. Alice gave permission to Bob for the entity.(Alice called approve(Bob's address, the entity's tokenId).)
4. Bob called nuke(the entity's tokenId).

In the scenario described above, it should be allowed for Bob to call nuke(the entity's tokenId). However, Bob's call will be reverted due to the bug highlighted in the report.

nftContract.getApproved(tokenId) returns Bob, not address(this). Additionally, msg.sender at L160 is Bob who have not approved the NukeFund contract as an operator. Consequently, the nuke() function will be reverted unexpectedly.

So, msg.sender at L160 should be replaced by nftContract.ownerOf(tokenId).

        require(
            nftContract.getApproved(tokenId) == address(this) ||
160:            nftContract.isApprovedForAll(msg.sender, address(this)),
                'Contract must be approved to transfer the NFT.'
            );

This unexpected reversal also takes place when Alice approves Bob as an operator in step 3 of the scenario.

Therefore, I believe this issue is valid.

Coded PoC

Add the following code into test/NukeFund.test.ts: In the PoC, Alice is the owner and Bob is the user1.

    it('should revert to nuke a token', async function () {
        const tokenId = 1;

        // Mint a token
        await nft.connect(owner).mintToken(merkleInfo.whitelist[0].proof, {
        value: ethers.parseEther('1'),
        });

        // Send some funds to the contract
        await user1.sendTransaction({
        to: await nukeFund.getAddress(),
        value: ethers.parseEther('1'),
        });

        const prevNukeFundBal = await nukeFund.getFundBalance();
        // Ensure the token can be nuked
        expect(await nukeFund.canTokenBeNuked(tokenId)).to.be.true;

        const prevUserEthBalance = await ethers.provider.getBalance(
        await owner.getAddress()
        );
        // await nft.connect(owner).approve(await nukeFund.getAddress(), tokenId);
        await nft.connect(owner).approve(user1, tokenId);
        await nft.connect(owner).setApprovalForAll(await nukeFund.getAddress(), true);

        const finalNukeFactor = await nukeFund.calculateNukeFactor(tokenId);
        const fund = await nukeFund.getFundBalance();

        // await expect(nukeFund.connect(owner).nuke(tokenId))
        await expect(nukeFund.connect(user1).nuke(tokenId)).to.be.revertedWith("Contract must be approved to transfer the NFT.");
    });

[Bauchibred]

Agree with @KupiaSecAdmin's PJQA comment. To add to what's already stated, my report from the validation repo also explains how nuking is currently broken for approved personnels due to the wrong check and should be duplicated to this: https://github.com/code-423n4/2024-07-traitforge-validation/issues/304

[koolexcrypto]

To summarize this, Alice approves Bob, Bob calls nuke but it reverts because it expect Bob to approve the contract which shouldn't be a requirement.

I believe this is a valid issue due to the broken functionality.

[koolexcrypto]

would be appreciated, If you have a dup of this, please add it on your own issue, not here.

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as satisfactory

[koolexcrypto]

@c4-judge +1 on this and I believe its worth looking into. My report also holds a coded POC that shows the same behaviour.

which one is your report ?

[c4-judge]

koolexcrypto marked the issue as selected for report