DatingDapp
AI First Flight #6
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

[M-1] Function `LikeRegistry::likeUser` should update the mapping `LikeRegistry::userBalance` with `msg.sender`'s `msg.value`, but it doesn't

datadata

[M-1] Function LikeRegistry::likeUser should update the mapping LikeRegistry::userBalance with msg.sender's msg.value, but it doesn't

Description

At no point in the function LikeRegistry::likeUser does the mapping LikeRegistry::userBalance get incremented by the value sent to it. This is the function that should handle the logic of updating the mapping. It conditionally calls LikeRegistry::matchRewards, which expects non-zero values noted in entries in userBalance.

function likeUser(address liked) external payable {
require(msg.value >= 1 ether, "Must send at least 1 ETH");
require(!likes[msg.sender][liked], "Already liked");
require(msg.sender != liked, "Cannot like yourself");
require(
profileNFT.profileToToken(msg.sender) != 0,
"Must have a profile NFT"
);
require(
profileNFT.profileToToken(liked) != 0,
"Liked user must have a profile NFT"
);
​
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
​
// Check if mutual like
if (likes[liked][msg.sender]) {
matches[msg.sender].push(liked);
matches[liked].push(msg.sender);
emit Matched(msg.sender, liked);
@> matchRewards(liked, msg.sender);
}
}
​
function matchRewards(address from, address to) internal {
@> uint256 matchUserOne = userBalances[from];
@> uint256 matchUserTwo = userBalances[to];
userBalances[from] = 0;
userBalances[to] = 0;
​
uint256 totalRewards = matchUserOne + matchUserTwo;
uint256 matchingFees = (totalRewards * FIXEDFEE) / 100;
uint256 rewards = totalRewards - matchingFees;
totalFees += matchingFees;
​
// Deploy a MultiSig contract for the matched users
MultiSigWallet multiSigWallet = new MultiSigWallet(from, to);
​
// Send ETH to the deployed multisig wallet
(bool success, ) = payable(address(multiSigWallet)).call{
value: rewards
}("");
require(success, "Transfer failed");
}

Likelihood:

This error will happen every time LikeRegistry::likeUser is called.

Impact

The impact is that when likeUser conditionally calls LikeRegistry::matchRewards, it will be looking up and retrieving the default value of a uint256, which is 0, when it defines local variables matchUserOne and matchUserTwo.

Risk

This will happen every time likeUser is called. The risk is that other functions like LikeRegistry::matchRewards will not work as expected.

Proof of Concept

We use the test suite's contract TestLikeRegistry, and add some storage variables and a modifier to have the users mint a profileNFT for themselves. We have written a test that proves that a user calling LikeRegistry::likeUser on another user does not affect values in mapping LikeRegistry::userBalances for either user.

contract testLikeRegistry is Test {
SoulboundProfileNFT soulboundProfileNFT;
LikeRegistry likeRegistry;
​
address owner = address(this); // Test contract acts as the owner of likeRegistry
address user1 = makeAddr("user1");
address user2 = makeAddr("user2");
uint256 public s_feeAmount;
uint256 public s_spendingFunds;
​
function setUp() public {
soulboundProfileNFT = new SoulboundProfileNFT();
likeRegistry = new LikeRegistry(address(soulboundProfileNFT));
s_feeAmount = 1 ether;
s_spendingFunds = 20 ether;
vm.deal(user1, s_spendingFunds);
vm.deal(user2, s_spendingFunds);
}
​
modifier mintProfilesUser1AndUser2() {
// prank user1 and user2 minting a profile for themselves
vm.prank(user1);
soulboundProfileNFT.mintProfile("user1", 23, "ipfs://profileImage");
vm.prank(user2);
soulboundProfileNFT.mintProfile("user2", 25, "ipfs://profileImage2");
_;
}
​
// test that LikeRegistry::likeUser does not increase userBalance for the liker nor liked
// This test passes.
function testLikeUserDoesNotUpdateUserBalances()
public
mintProfilesUser1AndUser2
{
// ARRANGE
uint256 user1BalanceBefore = likeRegistry.userBalances(user1);
uint256 user2BalanceBefore = likeRegistry.userBalances(user2);
​
// ACT
vm.prank(user1);
likeRegistry.likeUser{value: s_feeAmount}(user2);
​
// ASSERT
uint256 user1BalanceAfter = likeRegistry.userBalances(user1);
uint256 user2BalanceAfter = likeRegistry.userBalances(user2);
console.log("user1 balance after: ", user1BalanceAfter);
console.log("user2 balance after: ", user2BalanceAfter);
assertEq(user1BalanceBefore, 0);
assertEq(user2BalanceBefore, 0);
assertEq(user1BalanceBefore, user1BalanceAfter);
assertEq(user2BalanceBefore, user2BalanceAfter);
}
}

Recommended Mitigation

We recommend that the function LikeRegistry::likeUser updates msg.sender's entry in userBalances by adding msg.value to it.

function likeUser(address liked) external payable {
...
likes[msg.sender][liked] = true;
+ userBalances[msg.sender] += msg.value;
emit Liked(msg.sender, liked);
...
}

The project's README is ambiguous in explaining how the fees are extracted: all their previous like payments (minus a 10% fee) are pooled into a shared multisig wallet, which both users can access for their first date. We will assume that this includes the entirety of a user's entry in userBalance and not just for liking a different specific user.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 13 days ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-01] After the user calls the `likeUser` function, the userBalance does not increase by the corresponding value.

## Description User A calls `likeUser` and sends `value > 1` ETH. According to the design of DatingDapp, the amount for user A should be accumulated by `userBalances`. Otherwise, in the subsequent calculations, the balance for each user will be 0. ## Vulnerability Details When User A calls `likeUser`, the accumulation of `userBalances` is not performed. ```solidity function likeUser( address liked ) external payable { require(msg.value >= 1 ether, "Must send at least 1 ETH"); require(!likes[msg.sender][liked], "Already liked"); require(msg.sender != liked, "Cannot like yourself"); require(profileNFT.profileToToken(msg.sender) != 0, "Must have a profile NFT"); require(profileNFT.profileToToken(liked) != 0, "Liked user must have a profile NFT"); likes[msg.sender][liked] = true; emit Liked(msg.sender, liked); // Check if mutual like if (likes[liked][msg.sender]) { matches[msg.sender].push(liked); matches[liked].push(msg.sender); emit Matched(msg.sender, liked); matchRewards(liked, msg.sender); } } ``` This will result in `totalRewards` always being 0, affecting all subsequent calculations: ```solidity uint256 totalRewards = matchUserOne + matchUserTwo; uint256 matchingFees = (totalRewards * FIXEDFEE ) / 100; uint256 rewards = totalRewards - matchingFees; totalFees += matchingFees; ``` ## POC ```solidity function testUserBalanceshouldIncreaseAfterLike() public { vm.prank(user1); likeRegistry.likeUser{value: 20 ether}(user2); assertEq(likeRegistry.userBalances(user1), 20 ether, "User1 balance should be 20 ether"); } ``` Then we will get an error: ```shell [FAIL: User1 balance should be 20 ether: 0 != 20000000000000000000] ``` ## Impact - Users will be unable to receive rewards. - The contract owner will also be unable to withdraw ETH from the contract. ## Recommendations Add processing for `userBalances` in the `likeUser` function: ```diff function likeUser( address liked ) external payable { require(msg.value >= 1 ether, "Must send at least 1 ETH"); require(!likes[msg.sender][liked], "Already liked"); require(msg.sender != liked, "Cannot like yourself"); require(profileNFT.profileToToken(msg.sender) != 0, "Must have a profile NFT"); require(profileNFT.profileToToken(liked) != 0, "Liked user must have a profile NFT"); likes[msg.sender][liked] = true; + userBalances[msg.sender] += msg.value; emit Liked(msg.sender, liked); [...] } ```

Support

FAQs

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

Give us feedback!