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

`Soulmate::ownerToId` returns 0 as id for users who are not in relation leads to certain issues associated with other contracts.

shikhar229169

Summary

Soulmate::ownerToId returns the soulmate NFT id that is associated with the soulmate participating in the contract but it return 0 as id if the user has not participated in the protocol (i.e. has no soulmate).
As the token id starts from 0, thus token id - 0 is actually assigned to soulmates, but Soulmate::ownerToId returning 0 for non-soulmates user lead to potential loss of soulmates whose id is actually 0 as well as allows the non-soulmates users to enjoy the same benefits (of Airdrop & Staking), soulmates with id = 0 are enjoying.

Vulnerability Details

The vulnerability occurs due to starting tokenId for Soulmate NFT from 0 in Soulmate contract as Soulmate::ownerToId returns 0 for non-soulmate users.
Thus, these non-soulmate users can hinder with the soulmates with id = 0 with malicious intent and can cause danger to their relation (in worst case may also lead to their divorce) as well as allows them to take benefit of Airdrop as well as Staking without having any soulmates.

Impact

1. Airdrop.sol

Airdrop::claim function uses Soulmate::ownerToId to calculate the numberOfDaysInCouple as follows:

// Calculating since how long soulmates are reunited
uint256 numberOfDaysInCouple = (block.timestamp -
soulmateContract.idToCreationTimestamp(
soulmateContract.ownerToId(msg.sender)
)) / daysInSecond;

For non-soulmate users calling claim function, ownerToId will be evaluated as 0, thus the numberOfDaysInCouple is calculated considering them as soulmates with id = 0, allowing all non-soulmate users to claim Airdrop.

2. Soulmate.sol

Soulmate::writeMessageInSharedSpace allows soulmates to share messages with them and should not allow non-soulmate users or other soulmates to hinder with their messaging.
But ownerToId returning 0 for all non-soulmate users will allow them to hinder and manipulate the messages of soulmates with id = 0.
Therefore, allowing malicious users to put up hate messages in the shared space of soulmates with id = 0 and they can end up divorcing due to other malicious users circulating hate messages in their shared space.

function writeMessageInSharedSpace(string calldata message) external {
@> uint256 id = ownerToId[msg.sender]; <--- Returns 0 for non-soulmate users
sharedSpace[id] = message; <--- Updates the message for id = 0, even if the caller is not a associated soulmate with id = 0
emit MessageWrittenInSharedSpace(id, message);
}

3. Staking.sol

Staking::claimRewards allows users to claim rewards for their staked amount, but soulmateId calculated for the caller evaluates to 0 for non-soulmate users giving them benefits even if they have not staked their amount for a week or more.

PoC

Add the below test in file: test/unit/AirdropTest.t.sol and also import console2 from BaseTest.t.sol inside the AirdropTest.t.sol

Run the test:

forge test --mt test_UsersCanReceiveAirdropWithoutHavingASoulmate -vv
function test_UsersCanReceiveAirdropWithoutHavingASoulmate() public {
vm.prank(soulmate1);
soulmateContract.mintSoulmateToken(); // 💖
​
// 10 days later another user arrives
vm.warp(block.timestamp + 10 days);
​
vm.prank(soulmate2);
soulmateContract.mintSoulmateToken(); // ❤️
​
// here this person is not in relation (doesn't hold any soulbound nft)
address single = makeAddr("single");
​
// 5 days later
vm.warp(block.timestamp + 5 days);
​
assertEq(loveToken.balanceOf(single), 0);
​
// even though the person is single but still is able to claim Airdrop
vm.prank(single);
airdropContract.claim();
​
assert(loveToken.balanceOf(single) > 0);
​
// reason: as for people not in relation, the `ownerToId` returns 0 as token id
// and the creation timestamp of token id 0 will be used, thus user will receive: 15 - 10 = 5 tokens
console2.log("Love Tokens Of User who is Single received from Airdrop Claim:", loveToken.balanceOf(single));
}

Add the below test in the file: test/unit/SoulmateTest.t.sol

Run the test:

forge test --mt test_AllowsUserToHinderWithTokenZeroMessage -vv
function test_AllowsUserToHinderWithTokenZeroMessage() public {
uint256 id = soulmateContract.totalSupply();
​
vm.prank(soulmate1);
soulmateContract.mintSoulmateToken();
​
vm.prank(soulmate2);
soulmateContract.mintSoulmateToken();
​
assertEq(soulmateContract.ownerToId(soulmate1), id);
assertEq(soulmateContract.ownerToId(soulmate2), id);
​
address hater = makeAddr("hater");
​
vm.prank(hater);
soulmateContract.writeMessageInSharedSpace("I am seeing other soulmate these days and he drives a Porche! Bye-Bye");
​
vm.prank(soulmate2);
string memory message = soulmateContract.readMessageInSharedSpace();
console2.log(message);
}

Output with logs

Running 1 test for test/unit/SoulmateTest.t.sol:SoulmateTest
[PASS] test_AllowsUserToHinderWithTokenZeroMessage() (gas: 313451)
Logs:
I am seeing other soulmate these days and he drives a Porche! Bye-Bye, darling <----
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.08ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review, Unit Test in Foundry

Recommendations

Start token id from 1 instead of 0 and along with that Soulmate::ownerToId should revert if it is 0.

  • At line 32 in Soulmate

- uint256 private nextID;
+ uint256 private nextID = 1;

  • Make the mapping declaration private at line 26 in Soulmate

- mapping(address owner => uint256 id) public ownerToId;
+ mapping(address owner => uint256 id) private _ownerToId;

  • As we have changed the name from ownerToId to _ownerToId (added a underscore), thus update all the positions inside Soulmate contract where it is updated (at line 72 and 77)

- ownerToId[msg.sender] = nextID;
+ _ownerToId[msg.sender] = nextID;

  • Implement the actual function with name ownerToId which reverts when the user doesn't have any token (i.e. their id = 0)

error Soulmate__UserNotFound();
​
function ownerToId(address user) public view returns (uint256) {
if (ownerToId[user] == 0) {
revert Soulmate__UserNotFound();
}
return ownerToId[user];
}
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.

finding-self-soulmate

- Given the native anonymous nature of blockchain in general, this issue cannot be avoided unless an explicit whitelist is implemented. Even then we can only confirm soulmates are distinct individuals via kyc. I believe finding a soulmate is intended to be permisionless. - However, even though sufficient (500_000_000e18 in each vault) tokens are minted to claim staking and airdrop rewards, it would take 500_000_000 / 2 combined weeks for airdrop vault to be drained which is not unreasonable given there are [80+ million existing wallets](https://coinweb.com/trends/how-many-crypto-wallets-are-there/). Given there is no option to mint new love tokens, this would actually ruin the functionality of the protocol of finding soulmates and shift the focus to abusing a sybil attack to farming airdrops instead. Assigning medium severity for now but am open for appeals otherwise, since most if not all issues lack indepth analysis of the issue.

finding-write-message-nft-0-id

Medium Severity, This has an indirect impact and influence on the possibility of divorce between soulmates owning the first soulmate NFT id0, leading to permanent loss of ability to earn airdrops/staking rewards.

finding-claimRewards-nft-0-lastClaim

High severity, as it allows any pending user to claim staking rewards without owning a soulmate NFT by - Obtaining love tokens on secondary markets - Transfer previously accrued love tokens via airdrops/rewards to another account and abusing the `deposit()` function

Support

FAQs

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