code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L142-L145 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L174-L181 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L215-L223

Vulnerability details

Impact

While it is nice to have creator to approve right after minting in the same transaction, it also creates a phishing attack opportunity for an attacker. An attacker can phish creator/owner to call any of the mint and approve functions with an already prepared input while the victim is unaware.

Such phishing activity could allow the attacker to steal the collection NFTs the creator/owner has minted, also allows attacker to be set as the tokenCreatorPaymentAddress for royalties to steal the royalties as well.

Proof of Concept

Alice is the creator of collection NFT token
Alice wants to mint the NFT token
Bob knowing that Alice has created yet to mint the collection phishes Alice making Alice think she is only calling the mint() function.
However, Bob has set up the attack in such a way that it calls mintWithCreatorPaymentAddressAndApprove() instead with hardcoded operator and tokenCreatorPaymentAddress params . 5 . Alice surely mints the NFTs as planned unknowingly approving Bob and setting Bob as the royalty recipient
Bob transfers the collection NFTs Alice has minted and sells them right away while at same time receiving the royalties as well.

Tools Used

Manual review

Recommended Mitigation Steps

It may be necessary to seperate the call to approve operator.

ghost commented 2 years ago

The attack assumes that the victim (minter) signs a malicious payload generated by the attacker.

HardlyDifficult commented 2 years ago

ACK.

We believe this is Low risk. Phishing is an inherit risk when using blockchains.

We strive to ensure our contracts include appropriate separation of concerns and that we do not assume things like if they use the collection contract we created then they want to our the market contracts we have. Each of our contracts are standalone and could be used with or without our other contracts. So for approvals, we don't give our marketplace special treatment -- if approving while minting is a user experience improvement for our marketplace than it could be applicable for other marketplaces as well. e.g. this allows the user to mint and approve Seaport if that's their marketplace of choice.

Phishing is always a risk in this space. But if a frontend is going to do a phishing attack, it seems like there are more direct and profitable ways of targeting users than the path described here.

HickupHH3 commented 2 years ago

Phishing is always a risk in this space. But if a frontend is going to do a phishing attack, it seems like there are more direct and profitable ways of targeting users than the path described here.

Agree with sponsor. If a phishing attack is performed, it would be more profitable to get the user to sign a more malicious payload (eg. infinite approval to their most valuable ERC20 / NFT holdings).