Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

unforgiven - during migration, attacker can sell his NFT tokens in L1 after migration snapshot and then receive it in L2 after migration #144

Closed
sherlock-admin opened this issue Dec 1, 2023 · 4 comments
Labels
Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Dec 1, 2023

unforgiven

high

during migration, attacker can sell his NFT tokens in L1 after migration snapshot and then receive it in L2 after migration

Summary

DAO owner can migrate it to the L2, for migration a snapshot of token holder in L1 is taken then that snapshot will be send to L2 to deploy the new DAO in L2. the issue is that DAO Token contract is not pausable, so during the migration, attacker which owns DAO NFT, after the snapshot is taken, can sell his NFT in L1 and later receive his NFT in L2. This will cause loss to any user that buys or receive the NFTs during the migration. DAO NFT Token should have pause feature and DAO should be able to pause the token during the migration so no one could transfer it during the migration and after it.

Vulnerability Detail

This is the migration process:
image

attacker can exploit this process because during the migration and after the snapshot of NFT ownership is taken, DAO NFT token is transferrable and attacker can sell his token in L1 and later receive that token in L2. DAO owner can pause Auction, but that will only stop minting new tokens and it won't stop Token transfers.

warning users about the migration and saying that L1 NFT is going to be worthless is not gonna solve this issue, because there are a lot of DEX and on-chain marketplaces that attacker can sell its NFT during migration in L1.

Impact

users who buys or receives NFT tokens during migration in L1 will loss their funds.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/deployers/L2MigrationDeployer.sol#L92-L123
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/auction/Auction.sol#L362-L367

Tool used

Manual Review

Recommendation

add pause feature to DAO NFT Token, and allow owner to pause the DAO Token during the migration.

@github-actions github-actions bot added the High A valid High severity issue label Dec 6, 2023
@neokry neokry added the Sponsor Disputed The sponsor disputed this issue's validity label Dec 6, 2023
@neokry
Copy link

neokry commented Dec 6, 2023

There is almost no secondary market for any of the DAOs currently deployed on L1. We think this situation is pretty unlikely

@nevillehuang
Copy link
Collaborator

nevillehuang commented Dec 8, 2023

Situation is unlikely, but possible to allow potential unintended profit from token sales due to migration, so I believe this issue is definitely worth consideration.

Based on the following comments here made in the recent notional contest + sherlock docs revolving around future issues, incline to keep as medium since this is certainly an unintended future situation not mentioned in the Docs/READ.ME

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

However, I really don't understand the stance of sherlock's rules revolving future type issues such as this and #203 since it is a topic of relative gray area.

The difference between this issue and #203 is that in the latter, the code implementation already indicates that re-minting and burning logic is possible, whereas this is dependent on setting up of external secondary markets out of protocols control to maliciously act on. Seeking for further clarification here @Czar102, your help will be much appreciated.

@Czar102
Copy link

Czar102 commented Dec 11, 2023

I think this issue is invalid. If someone is proposing to buy a token and they pay for it after the migration, then they either see value in the token despite the loss of protocol functionality, or they made a mistake by failing to check that the token is still useful.
Decisions made by buyers on secondary markets are out of scope.

users who buys or receives NFT tokens during migration in L1 will loss their funds.

It't not true, they will receive a token on the L1. The buyer shouldn't assume that this token has utility without checking that a migration hasn't started ("actions are paused" per sponsor comment).

Please let me know if my stance makes sense in the context of the protocol @nevillehuang.

@nevillehuang
Copy link
Collaborator

nevillehuang commented Dec 11, 2023

@Czar102 I agree with this stance, thanks for the review

@nevillehuang nevillehuang removed the Medium A valid Medium severity issue label Dec 13, 2023
@sherlock-admin2 sherlock-admin2 changed the title Skinny Frost Puppy - during migration, attacker can sell his NFT tokens in L1 after migration snapshot and then receive it in L2 after migration unforgiven - during migration, attacker can sell his NFT tokens in L1 after migration snapshot and then receive it in L2 after migration Dec 13, 2023
@sherlock-admin2 sherlock-admin2 added the Non-Reward This issue will not receive a payout label Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Non-Reward This issue will not receive a payout Sponsor Disputed The sponsor disputed this issue's validity
Projects
None yet
Development

No branches or pull requests

5 participants