6529-Collections / nftdelegation

Delegation contract
MIT License
16 stars 7 forks source link

Fraudulent Delegations #5

Closed 0xCooki closed 1 year ago

0xCooki commented 1 year ago

Let me begin by stating that I really like the overall idea and goal. In particular, the lack of external dependencies is a highly admirable design constraint. With that being said, I perceive a non-trivial issue with the design of this contract in its current form; which is that there does not seem to be any method for identifying legitimate from illegitimate delegations in a trustless manner.

To see this, notice that because the registerDelegationAddress function is public, and because no assumptions are made regarding the kind of NFT that is being delegated (i.e. is it an ERC721 or an 1155? etc.), there is no require clause in the function that ensures that only owners of the NFT can delegate. It is recognised that a primary goal of this contract is to enable NFTs in a vault to be delegated without exposing the vault to signatures or transactions that malicious actors might abuse.

All of this means, however, that anyone can delegate any NFT to any other address with any given use case. Not only does this enable the rightful owner of an NFT to delegate their NFT using, let's say, a hot wallet in a manner they desire, but it also means that anyone else can do likewise. Crucially, there is no method for determining which of these delegations are legitimate as opposed to illegitimate. To help see this, consider the following example:

Two unsatisfying solutions to this problem come to mind, but both unfortunately require exposing the vault where the NFT is located to signatures that might be abused by bad actors. Firstly, the contract can be built to require that only owners of the NFT can delegate, but obviously this is unideal. Secondly, the current system can be maintained with the addition of some form of attestation that enables the owner of the NFT to "sign-off" on a specific delegation in order to distinguish it from any potential illegitimate delegations. Unfortunately this is equally unideal for the same reasons.

If I am miss-understanding a critical component of how the contract, or it's wider use, will operate I sincerely apologise and this entire critique can be ignored. Furthermore, if I can help in any way don't hesitate to let me know; I love tinkering with these sorts of contracts.

All the best, Cooki

Psifour commented 1 year ago

This is a non-issue as it assumes that the purpose of delegation is to delegate individual NFTs within a collection. Instead, delegation is performed on a collection-level in a simple to<->from arrangement.

To use your example, both Punk6529 and Cooki can delegate the relevant collection and said delegation makes no assertion to what NFTs are held by the "to" address in the mapping. This means that the "chromie squiggle" would still be safe in Punk6529's wallet and anyone looking at the delegations would see a delegation for the collection by Cooki, but a subsequent lookup would show no chromie squiggle in their wallet.

Also, the function you reference, registerDelegationAddress, would require a signature by Punk6529 and Cooki when they were registering their delegations. This is tied to the msg.sender address that is used in two out of the three hashes that are created by said function.

TL;DR: Delegation is collection-wide and makes no assertions about any users ownership in said collection.

0xCooki commented 1 year ago

Whether delegations are made in relation to whole collections or specific NFTs is irrelevant to the issue I'm raising. I used an example of a specific NFT because I thought it was more intuitive, and because in the twitter thread which made me aware of this project it was made clear that the ability to delegate specific NFTs would be integrated in the future also.

Also, the function you reference, registerDelegationAddress, would require a signature by Punk6529 and Cooki when they were registering their delegations. This is tied to the msg.sender address that is used in two out of the three hashes that are created by said function.

This is precisely my point; the contract doesn't require a signature by Punk6529 and Cooki when they were registering their delegations. Anyone can make a delegation for any NFT or collection. In order to determine the legitimate from the illegitimate delegations others must, as you say, check to see if the msg.sender of the initial delegation actually owns the NFT. The issue is that the contract itself has no method for distinguishing between the legitimate and illegitimate delegations. It must be others, if they are using the registry to confer rights and responsibilities, that validate the entries before use.

All registries in the contract must therefore be treated as unverified by users unless and until any given delegation is verified by independently querying the owner/s of the NFT/s in question. At the very least this is counter-intuitive and it should be made clear to users that the mere existence of a delegation in the contract does not imply that the delegation is legitimate. Inevitably, among the set of all delegations in the registry only a subset will be legitimate (in the sense that the delegator owns the NFT being delegated).

Psifour commented 1 year ago

Anyone can make a delegation for any NFT or collection.

This is the line that helped me understand your concern. You are correct that any address can (and should be able to) create a delegation for any collection. This is exactly the behavior that is desired. I don't own a CryptoPunk, but I can easily delegate that collection. I can even delegate Punk #6529 if I desire.

These delegations will exist, but be meaningless if I don't own any of the underlying assets. This doesn't make them 'illegitimate' so much as 'non-applicable'. What I mean by that is that if I (User A) "delegate" my Punk(s) to User B it has no meaningful purpose as I (User A) will still not own any Punks. Delegation is NOT ownership and there is no viable way to make it perform as such. At best you could (theoretically) create a mirror of the underlying collection and at worst a mess of "ownership" state that will act as a lagging index into the 'true' state.

Ownership should always remain the purview of the collections own contract(s) and delegation should be an abstraction that only cares about having an accurate on-chain method for establishing non-custodial A->B relationships. If you felt that delegation had a stronger/different connotation than perhaps there is a need for project scope/documentation to further define the what this is/isn't intended to do. It is also possible that I am the one who has misunderstood scope, but I don't believe that is the case based on existing documentation.

0xwurdig commented 1 year ago

@CookedCookee is absolutely right about how anyone can delegate anyone for any collection for any useCase.

However, the magic of this contract doesn't lie in registerDelegationAddress() but in retrieveActiveToDelegations()

Let's take a look at a few examples:

Example 1:

Someone (Bob) wants to make a derivative work like @AvidLines but for Fidenzas.

Bob would call retrieveActiveToDelegations() for every Fidenza holder's address( for example, 6529museum for The Tulip), along with ArtBlock's contract address and the useCase id required for signing.

The returned array shall contain the address(s) Bob needs to whitelist so that the txn can be signed with a wallet delegated by 6529museum for that specific useCase. Hence, no exposure to the asset.


Example 2:

The BAYC want to airdrop a token (I am not saying they do, but if they did).

They could again call retrieveActiveToDelegations() for every BAYC holder's address along with the collection address and the useCase id (3 for airdrop)

The returned array shall provide BAYC with addresses the holder wishes to be airdropped in (if any) and then they can choose the latest delegation and go ahead with the airdrop.


Example 3:

Someone wants to make a game for Wicked Craniums

retrieveActiveToDelegations() for every Cranium holder's address, collection address and the useCase id (12 for gameplay)

The returned array shall provide... Well, I guess that sums it up.

So no matter the number of delegations registered with registerDelegationAddress(), only the delegations made by owner address will be applicable with the help of retrieveActiveToDelegations()

As @Psifour stated:

These delegations will exist, but be meaningless if I don't own any of the underlying assets. This doesn't make them 'illegitimate' so much as 'non-applicable'.


Regarding the additional parameter for tokenID:

A parameter for tokenID could be a great addition but at the same time would just make things complex for the end user, for example, a full set 6529Meme holder will have to sign multiple times to be able to access derivatives/airdrops from different artists rather than just once.

On the contrary makes sense if, for example, you and your kid want to play the new Wicked Craniums game but with individual progress (kind of like different profiles in current games) So you can delegate different wallets for gameplay for different tokenIDs.

Guess, this contract will take shape further trying to satisfy the good of both worlds.

0xCooki commented 1 year ago

Bob would call retrieveActiveToDelegations() for every Fidenza holder's address( for example, 6529museum for The Tulip), along with ArtBlock's contract address and the useCase id required for signing.

Got it! that makes sense to me. Cheers everyone!

brookr commented 1 year ago

Interesting clarifications. I suppose @CookedCookee, if you agree there is no cause for concern here, you can close the issue?