Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Token ownership check during the raffle initialization is meaningless #192

Closed
code423n4 opened this issue Dec 16, 2022 · 2 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-88 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L126

Vulnerability details

Impact

It is not guaranteed that the raffle owner is the actual owner of the token up for a raffle.

Proof of Concept

And whenever a new raffle is created, msg.sender is used for initialization and it is checked if the caller is actually the owner of the token.
From this, I believe that the protocol intended to guarantee that the raffle owner actually owns the token up for a raffle.

VRFNFTRandomDraw.sol
75:     function initialize(address admin, Settings memory _settings)
76:         public
77:         initializer
78:     {
...
124:
125:         // Get owner of raffled tokenId and ensure the current owner is the admin
126:         try
127:             IERC721EnumerableUpgradeable(_settings.token).ownerOf(
128:                 _settings.tokenId
129:             )
130:         returns (address nftOwner) {
131:             // Check if address is the admin address
132:             if (nftOwner != admin) {
133:                 revert DOES_NOT_OWN_NFT();
134:             }
135:         } catch {
136:             revert TOKEN_BEING_OFFERED_NEEDS_TO_EXIST();
137:         }
138:     }

But this guarantee can be easily broken by transferring the ownership of the raffle.
This means a user can create unlimited number of raffles for the same token and transfer ownership to others.
Although the other owners can not start a draw, the impact might differ according to the business logic in the sense of how the ownership of a raffle for a token is valued.
I believe this is not what the protocol intended.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider locking the NFT token during the initialization.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 16, 2022
code423n4 added a commit that referenced this issue Dec 16, 2022
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as duplicate of #104

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-88 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants