First Flight #9: Soulmate

Summary

The majority of vulnerabilities I found are related to validation, including validation on users as well as validation on vault balances. Additionally, an issue was found on the default value of nextID when initialized without an explicit value.

Vulnerability Details

Soulmate

• nextID initialized without an explicit value in Solidity will default its value to 0 instead of null/None. Can be a huge vulnerability, including in the claim function within Airdrop.sol. For example, the value of numberOfDaysInCouple effectively becomes block.timestamp / daysInSecond since the value of soulmateContract.idToCreationTimestamp(soulmateContract.ownerToId(msg.sender)) will become 0 if msg.sender is not found in the Soulmate contract. An unknown user could exploit this and gain loveToken in the amount of block.timestamp / daysInSecond

• if isDivorced, should be disabled from re-minting with SoulmateToken, writing message with writeMessageInSharedSpace, and being able to re-run getDivorced function

• sharedSpace visible by others, but this should ideally only be visible by the soulmates it belongs to

Airdrop

• The above Soulmate vulnerability with nextID goes hand-in-hand with user validation that is required within the claim function to revert when a user who never attempted to find a soulmate, or a user who is still looking for a soulmate is trying to run the function. If no user validation occurs, the claim function will proceed to transfer block.timestamp / daysInSecond in loveToken to msg.sender

• Should revert/discontinue function if Airdrop vault is empty

Staking

• Staking needs dust collector like in Airdrop to handle insufficient funds

• Should revert/discontinue function if Staking vault is empty

Impact

• The greatest impact occurs within the claim function from the Airdrop contract where an invalid user without a soulmate can transfer block.timestamp / daysInSecond into his or her own wallet.

• The lack of some type of dust collector that checks for insufficient funds within the staking vault can lead to users not being able to properly claim the funds owed to them

• Lack of validation on identity, funds, or isDivorced can lead to users access content they shouldn't have access to or a loss of gas with a function running until the end

Tools Used

• Forge/Cast

• VSCode

Recommendations

• nextID should be initialized to the value of 1. This used in conjunction with user validation at the beginning of many of the functions within the repo will ensure that in the case where ownerToId[msg.sender] evaluates to 0, this is treated like null/None and the function will be reverted. This can save gas and of course prevent the loss of funds to invalid users.

• user validation to check if msg.sender has been attempting to find a soulmate or if a soulmate has already been found.

  • Soulmate: if msg.sender hasn't found soulmate, should revert functions of writeMessageInSharedSpace, readMessageInSharedSpace, getDivorced, isDivorced

  • Airdrop: if msg.sender hasn't found soulmate, should revert function of claim

  • Staking: if msg.sender hasn't found soulmate, should revert functions of claimRewards

• Should prevent staking/airdrop functionalities if no fund are available, which would save gas

• Would recommend a dust collector for staking. Potentially even revert the function and ensure lastClaim[msg.sender] isn't updated yet to remember the correct amount owed

• Would recommend sharedSpace be secure and visible only to soulmates. Would probably need to use wallets to secure

NOTE

• While attempting to resolve this contest, I found that msg.sender does not necessarily carry the same value across contracts when running a particular function. For example, I found that if a particular test's contract calls another "X" contract, the msg.sender values within each contract may differ. The msg.sender value within the test's contract may be a particular value while the msg.sender value within the "X" contract will be the address to the test contract itself. I didn't expect this behavior and as a result, the particular solution with the helper functions checkIfSenderFound and checkIfSoulmateFound often fails due to a mismatch of addresses. I unfortunately haven't the knowledge or experience yet to navigate through this issue. I am eager to learn the proper method of handling this situation. As a result, most of the tests fail as of this moment.