Shungy
commented
2 years ago Leaf is generated using msg.sender: https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L347
You cannot claim for other's. No need to have an explicit check.
GalloDaSballo
Lines of code
https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L339-L357
Vulnerability details
Impact
A malicious user can claim and successfuly steal a gobbler NFT token in the function claimGobbler(). https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L339-L357
Proof of Concept
The function claimGobbler() is used from the mintlisted users to claim a gobbler using a merkle proof. However there is no check to ensure that the msg.sender providing the merkle proof is the real owner of it, so anyone who has access to the merkle proof can claim it. If for some reason one of the mintlisted users's merkle proof is leaked, a malicious user can claim and steal the gobbler NFT token, before the real owner of the merkle proof. And later when the real owner tries to claim the gobbler, the function will revert, because it was already claimed by the malicious user.
Tools Used
VS Code
Recommended Mitigation Steps
My recommended fix is: https://gist.github.com/CodingNameKiki/c460d7108903a829cf4e5bf7d786624c
// We create a new mapping (address => bool) IsMintListedUser; // The first function is the external function only called by the Admin. // Admin writes the Mintlisted users addresses and their bool is set as "true" with the mapping "IsMintListedUser". // The mapping shows a bool "true" only for the written addresses from the admin function.
// In the function claimGobbler(), if the right proof is provided and the msg.sender's bool is set as "true", it will pass! // If the right proof is provided, but the msg.sender's bool is "false", the function will revert. // After that we delete the msg.sender's bool, so that the function will revert if called again by the same address. // And after the msg.sender's bool is deleted, the address can be mintlisted and used again in the future. // The mapping "mapping(address => bool) public hasClaimedMintlistGobbler;" won't be needed, since the function // reverts with the new code if called second time by the same MintListed user. // And the merkleProof can't be used again, because it is one time claimable.
What will be the result after applying this changes? 1.There's no chance of stealing a gobbler token anymore. The gobbler tokens can be claimed only by the MintListed users with their merkle proof. 2.If someone has access to a merkle proof and isn't a MintListed user, the function will revert. 3.After a Mintlisted user claimed his gobbler, the function will delete his msg.sender's bool, and it will revert if called again by the same address. 4.By deleting the msg.sender's bool, the address can be mintlisted and used again in the future.