Submitted by KupiaSec, also found by Bauchibred and ZanyBonzy
Approved users cant nuke the owners NFT.
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
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
msg.sender at L143 is NukeFund contract. In the ERC721._isApprovedOrOwner, it checks the following:
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.
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).
The nuke function should check nftContract.isApprovedForAll(nftContract.ownerOf(tokenId), address(this)), instead of nftContract.isApprovedForAll(msg.sender, address(this)).
It is recommended to change the code as follows:
Access Control
KupiaSec (warden) commented:
Koolex (judge) commented:
