First Flight #9: Soulmate

Summary

The Soulmate::writeMessageInSharedSpace allows couples to write messages each other in shared space associated with their token id. The function relies on the ownerToId mapping to determine the id associated with the caller's address. If a caller who does not have a soulmate attempts to write a message, the ownerToId mapping should not have an entry for his address, and it should default to 0 since mappings in Solidity return the default value for the value type when accessed with a key that does not exist.

Vulnerability Details

The function Soulmate::writeMessageInSharedSpace allows the person to write message to the soulmate. The function is public and there is no check if the caller of the function has actually a soulmate. Therefore, people who don't have a soulmate can also calls the function and write a message in shared space.

The function Soulmate::writeMessageInSharedSpace get the id token of the caller (msg.sender) and assign a message to this id in the sharedSpace mapping.
If the caller is waiting for a soulmate, he/she will have a valid id and the message will be written to his/her id. the caller can write a message to the future soulmate.
But if the caller doesn't have a soulmate and his/her address doesn't exist in the ownerToId mapping, the return value from the ownerToId[msg.sender] will be 0. So the message will be written in the sharedSpace with id 0. The problem is that the id 0 is reserved for the first couple of the protocol. And this couple will receive message that is not intended to receive.
Also, the messages in the shared space are not saved and the new message overwrites the previous one.

Impact

Let's consider the following scenario. There is a couple who use the protocol (soulmate1 and soulmate2). Their token id is equal to 0. Then Bob who doesn't have a soulmate calls the function Soulmate::writeMessageInSharedSpace with a message and this message will be written to the sharedSpace with id equals to 0. Thus, the couple whose token id is equal to 0 will read this message and soulmate1 will think that the message is from soulmate2 and vice versa. That leads to confusion and dependency of the message to the problems in the couple.

The following test shows the possibility of writing message from a person who doesn't have a soulmate in the shared space of the existing couple. You can add the test in the test file Soulmate.t.sol and execute it with the command: forge test --match-test "test_writeMessage".

Additionally, the Soulmate::writeMessageInSharedSpace writes the new message over the last one. There is no mechanism to save the all messages between soulmates. In that case the person who doesn't have a soulmate and writes a message in the shared space with id equals to 0 will overwrite the last message between soulmates.
Let's consider the following scenario. Soulmate1 and Soulmate2 are a couple. Soulmate1 writes a message to a soulmate2 in a shared space with id equals to 0 to buy apples. But at the same time Bob who doesn't have a soulmate and doesn't wait for a soulmate calls the function and overwites the message from the soulmate2. So the soulmate2 will read a message with text Buy some eggs that is not written from a soulmate2. You can add the following test that demonstrates this scenario in the test file Soulmate.t.sol and execute it with the command: forge test --match-test "test_overwriteMessage".

Also, if the both soulmates write at the same time to each other, then they can read only the last message. That means some of the messages will be lost and that leads to a confusion in the chat between soulmates.
If the soulmate1 writes a message to the soulmate2 and the soulmate2 writes a message before reading the prevous one, the message from the soulmate1 will be lost. The following test shows this case:

Tools Used

VS Code, Foundry

Recommendations

Add a check in the Soulmate::writeMessageInSharedSpace function that the caller has a soulmate or is waiting for a soulmate.
One possible way is to check if the id is associated with the address of the caller:

If you run the test function after applying the changes, the test will fail:
[FAIL. Reason: revert: The caller doesn't have soulmate] test_writeMessage() (gas: 216077)

In order to save the whole chat history between the soulmates you can add a mapping that stores every message:
mapping(uint256 => string[]) public sharedSpaceHistory; And in the Soulmate::writeMessageInSharedSpace to add:
sharedSpaceHistory[id].push(message);.
But this approach has a disadvantage that storing a lot of data on the blockchain can become very expensive in terms of gas costs. If the chat history grows large, it could become prohibitively expensive to continue appending messages. An alternative approach could be to store the chat messages off-chain (e.g., on IPFS) and only store a reference (like a hash or URI) on-chain.