6529-Collections / nftdelegation

Delegation contract
MIT License
16 stars 7 forks source link

DOS stuffing attack against a collection? #15

Closed brookr closed 1 year ago

brookr commented 1 year ago

The globalHash is the hash of:

msg.sender, _collectionAddress, _delegationAddress, _useCase

So, a determined attacker (with resources) could call the registerDelegationAddress an unlimited number of times, and "stuff" the registeredDelegation hash (and the To hashes) with massive amounts of data, by invoking the function with random To addresses, but the same collection address and use case.

This would make retrieval functions for that collection return an unwieldy amount of data, or maybe time out due to gas limits? I'm not sure exactly what would happen if the allDelegations objects got very large.

This could be mitigated if the globalHash was based solely on:

msg.sender, _collectionAddress, _useCase

So the Delegator could only have one entry per use case for a given collection. Expiry updates would still need to be handled by first revoking, then creating a new delegation (see #13).

0xwurdig commented 1 year ago

Interesting scenario, but I don't see this as much of an issue unless delegations are retrieved for the attacker's address. Let me grab an example from #5

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.

Unless the attacker's address holds an ape, it won't matter. As it will never be read.

However, if an ape holder acts maliciously for some reason, then yes the above issue holds water.

In that case, I propose an alternate solution of having an upper limit for delegationToCounterPerHash[toHash] rather than restricting to having only one delegationAddress for a use case on collection/token.

Say 10 is the upper limit, you cant have more than 10 delegated wallets for a specific use case on a specific collection. (REDACTED)

EDIT:-

No change required

However, if an ape holder acts maliciously for some reason...

All good, the malicious holder's delegated address won't be fetched due to timeout or gas limits and hence BAYC will default to airdrop the holding address.

rookmate commented 1 year ago

I would say it is easier if you only have one wallet per use case for each From address. Your To mint wallet should only be one for that From address.

In my opinion edge cases and over complication are what have people create multiple "standards"

brookr commented 1 year ago

I would say it is easier if you only have one wallet per use case for each From address. Your To mint wallet should only be one for that From address.

I don't think people will maintain a different wallet for every use-case though. The average user might have just 2 wallets. One for cold storage, and delegated to one for anything risky.

brookr commented 1 year ago

Interesting scenario, but I don't see this as much of an issue unless delegations are retrieved for the attacker's address.

There are multiple functions in the code that loop over both delegateFromHashes[hash] and delegateToHashes[hash] for a given address, collection, and use case. If these can be stuffed with meaningless data, those loops will be very large, and could get prohibitively expensive with gas, so much so that they'd be unusable.

...rather than restricting to having only one delegationAddress for a use case on collection/token.

Why would anyone ever need more than a one delegationAddress for a single use case within a collection? If this contract returns multiple delegated addresses, the collection managers are going to have to sort out what to do with the multiple values.

All good, the malicious holder's delegated address won't be fetched due to timeout or gas limits and hence BAYC will default to airdrop the holding address.

This is a very bad experience for those managing the collection... if the gas limit is hit, the gas fees are lost—not refunded. Would a project keep using this delegation contract if they were losing gas fees on it over and over again?

We can, and should, prevent this kind of stuff attack.

rookmate commented 1 year ago

I would say it is easier if you only have one wallet per use case for each From address. Your To mint wallet should only be one for that From address.

I don't think people will maintain a different wallet for every use-case though. The average user might have just 2 wallets. One for cold storage, and delegated to one for anything risky.

Then you would agree that there would not be the need for a huge array of delegations right? You could filter the active From/To for that single wallet/collection/token only

And completely agree with your last comment

skyn3t2030 commented 1 year ago

Locks were added during the registration to tackle spam attacks on delegation addresses.

brookr commented 1 year ago

I see that the Locks are checked during the registration process, but how does this deter spam/stuffing attacks?