Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

Divorced soulmates can still `claim` the `airdrop`

Summary

When a soulmate is divorced and still tries to claim the airdrop, he will succeed because Airdrop::claim will never revert.

Vulnerability Details

When a soulmate is divorced and still tries to claim the airdrop, he will succeed because Airdrop::claim will never revert throwing Airdrop::Airdrop__CoupleIsDivorced error because Soulmate::isDivorced function will always return false because in this case msg.sender will always be Airdrop.sol.

Source Code

Airdrop.sol:
https://github.com/Cyfrin/2024-02-soulmate/blob/b3f9227942ffd5c443ce6bccaa980fea0304c38f/src/Airdrop.sol#L53

if (soulmateContract.isDivorced()) revert Airdrop__CoupleIsDivorced();

Soulmate.sol:
https://github.com/Cyfrin/2024-02-soulmate/blob/b3f9227942ffd5c443ce6bccaa980fea0304c38f/src/Soulmate.sol#L131C5-L133C6

function isDivorced() public view returns (bool) {
return divorced[msg.sender];
}

Impact

The soulmates who decide to get divorced and still after getting divorced tries to claim the pending airdrop tokens, they will be able to do that because the intended check is implemented incorrectly.

Proof of Concept

Code
function test_ClaimAirdropAfterDivorce() public {
address auditSoulmate1 = makeAddr("auditSoulmate1"); // creating address for solmate1
address auditSoulmate2 = makeAddr("auditSoulmate2"); // creating address for solmate2
vm.prank(auditSoulmate1);
soulmateContract.mintSoulmateToken(); // minting token first
vm.prank(auditSoulmate2);
soulmateContract.mintSoulmateToken(); // minting token second and soulmates reunited
vm.warp(block.timestamp + 200 days + 1 seconds); // advancing time by 200 days
vm.startPrank(auditSoulmate1);
soulmateContract.getDivorced(); // calling the method to get divorced. It's enough I DON'T WANT TO LIVE WITH YOU ANYMORE
airdropContract.claim(); // Ohh GOD! before getting divorced I forgot to claim my airdrop, SHITTTT!!, OK let's give it a try.... BOOOMMM!! After getting divorced, still able to claim the CLAIM!!! yeahhhh!!!
uint256 soulmate1BalanceAfterClaim = loveToken.balanceOf(auditSoulmate1); // balance of soulmate1 after claim
vm.stopPrank();
assertEq(soulmate1BalanceAfterClaim, 200 ether); // asertion to proove that claim was successful and token balance increased
}

Tools Used

Manual Review

Recommendations

In Airdrop::claim function, msg.sender should be passed as an argument to Soulmate::isDivorced function. In Soulmate::isDivorced function, the provided soulmate should be checked in Soulmate::divorced mapping for the status.

Diff

Airdrop.sol

- if (soulmateContract.isDivorced()) revert Airdrop__CoupleIsDivorced();
+ if (soulmateContract.isDivorced(msg.sender)) revert Airdrop__CoupleIsDivorced();

Soulmate.sol

- function isDivorced() public view returns (bool) {
+ function isDivorced(address _soulmate) public view returns (bool) {
- return divorced[msg.sender];
+ return divorced[_soulmate];
}
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-claim-airdrop-without-owning-NFT

High severity, This issue is separated from the flawed `isDivorced()` check presented in issue #168 as even if that is fixed, if ownership is not checked, isDivorced would still default to false and allow bypass to claim airdrops by posing as tokenId 0 in turn resulting in this [important check for token claim is bypassed.](https://github.com/Cyfrin/2024-02-soulmate/blob/b3f9227942ffd5c443ce6bccaa980fea0304c38f/src/Airdrop.sol#L61-L66). #220 is the most comprehensive issue as it correctly recognizes both issues existing within the same function.

0xe4669da Submitter
over 1 year ago
0xe4669da Submitter
over 1 year ago
0xnevi Lead Judge
over 1 year ago
0xe4669da Submitter
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-isDivorced-wrong-check

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.