SPGNFT with default disabled approvals

[Ramarti]

Description and context

Typically, crypto drainers work by tricking users into signing transactions that would empty their wallet of assets instead of their intended transaction. For ERC721 is typically done through the approve or permit method when available (as seen in the popular Inferno Drainer)

For projects in the Story eco where IP Registration is a more web2 experience transparent to the user, we can assume that approve related functionality is not important for them at first. With the prevalence of drainers in the crypto space, this becomes an attack vector for the unsuspecting user going to web3 UX (or even the project's backend handling wallets)

Lens-v2 introduced ProtocolGuardians to protect user profiles (which were NFTs)
https://github.com/lens-protocol/LIPs/blob/main/LIPs/lip-4.md

We could give the deployer of an SPGNFT the option to start with ERC721 approve disabled by default, with every user having the option to enable it when they needed (for example right before listing an ERC721 in a marketplace).

Suggested solution

I would favor a simple flag. Pseudocode:

contract SPNFTExplicitApproval is SPNFT {
  // ...
  mapping(address, bool) approvalEnabled; // default is false for every address

  modifier ifApprovalEnabled(uint256 id) {
    require(approvalEnabled[ownerOf(id)]), "approvalDisabled");
    _;
  }
  
  function approve(address to, uint256 tokenId) ifApprovalEnabled(tokenId) {
    super.approve(to, tokenId);
  }

  function enableApprovals(uint256 id) onlyOwner(id) {
    approvalEnabled[ownerOf(id)] = true;
  }

}

Since this adds an SLOAD to approve, some projects might chose to go with the regular SPNFT

[sebsadface]

Thanks for the suggestion @Ramarti! Adding an extra layer of protection definitely makes sense. One question tho, I’m wondering how much this actually helps, couldn’t an attacker just batch both calls and still pull off the attack?

[letsvitta]

Thanks for the suggestion! — adding an additional step to set approvals makes it harder for users to mistakenly approve malicious users or contracts.
However, this change requires applications to understand how to handle the additional function call, which might affect user interactions with existing protocols and applications (e.g. OpenSea). We might need more carefully on the impact this has on interoperability.

[Ramarti]

@sebsadface that is a great point. We could force 2 steps by storing block.number instead of bool in the mapping, and verifying that approvalEnabled[ownerOf(id)] > block.number. Batched txs should fail that check.

In fact, Lens implementation had 7 days cooldown period.

@letsvitta 100%. OpenSea (Color better :D ) would revert out of the box.
The projects using this option would need to explain it to the user and have dedicated UI to enable trading his token.

Most web3 projects may not care, I was thinking mostly on projects with a lot of web2 creators that may offer later an "exit" to web3. Part of the exit would be warning the users of drainers and giving the option to activate approve