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

Resetting `LikeRegistry::userBalances` mapping to `0` in `LikeRegistry::matchRewards` function can cause uneven fund distribution for users in multiple matches

syedasadkazmii

Description

In the LikeRegistry::matchRewards function, the LikeRegistry::userBalances mapping values are being reset to 0, which could result in the uneven distribution of funds especially in case of multiple matches having a common underlying user, by transferring lesser ether amount than expected to one of the MultiSigWallet contracts and more ether amount than expected to the other.

Impact

Uneven distribution of funds to the deployed MultiSigWallet contracts corresponding to the matches.

Proof of Concept

  1. Bob executes the LikeRegistry::likeUser function, passing the address of Alice and attaching 1 ether as msg.value.

  2. Now, the value of userBalances[Alice] is 1 ether.

  3. Jack also executes the LikeRegistry::likeUser function, passing the address of Alice and attaching 1 ether as msg.value.

  4. Now, the value of userBalances[Alice] is 2 ether.

  5. Alice executes the LikeRegistry::likeUser function, passing the address of Bob and attaching 1 ether as msg.value.

  6. Now, the value of userBalances[Bob] is 1 ether.

  7. Now, since the like of Bob-Alice is mutual, when Alice executes the LikeRegistry::likeUser function, the likes[liked][msg.sender] value will be true, triggering the call to the LikeRegistry::matchRewards function.

  8. In the LikeRegistry::matchRewards function, the value of totalRewards would be 3 ether (i.e., the sum of userBalances[Bob] and userBalances[Alice]), and the values of userBalances[Bob] and userBalances[Alice] would be reset to 0.

  9. The MultiSigWallet contract corresponding to Bob-Alice match (say MultiSigWallet_BobAlice) is deployed and funded with 3 ether minus 10% FEE (0.3 ether), that makes the balance of the MultiSigWallet_BobAlice as 2.7 ether.

  10. Alice executes the LikeRegistry::likeUser function, passing the address of Jack and attaching 1 ether as msg.value.

  11. Now, the value of userBalances[Jack] is 1 ether, but the value of userBalances[Alice] is 0.

  12. Now, since the like of Jack-Alice is mutual, when Alice executes the LikeRegistry::likeUser function, the likes[liked][msg.sender] value will be true, triggering the call to the LikeRegistry::matchRewards function.

  13. In the LikeRegistry::matchRewards function, the value of totalRewards would be 1 ether (i.e., the sum of userBalances[Jack] and userBalances[Alice]), and the value of userBalances[Jack] and userBalances[Alice] would be reset to 0.

  14. The MultiSigWallet contract corresponding to Jack-Alice match (say MultiSigWallet_JackAlice) is deployed and funded with 1 ether minus 10% FEE (0.1 ether), that makes the balance of the MultiSigWallet_JackAlice as 0.9 ether.

  15. Ideally, the balance of MultiSigWallet_BobAlice should be equal to MultiSigWallet_JackAlice, with both being 1.8 ether. However, as seen above, the balance of MultiSigWallet_BobAlice is 2.7 ether, while the balance of MultiSigWallet_JackAlice is only 0.9 ether.

  16. All of this happened due to the uneven distribution of funds resulting from Alice being the common underlying user in both the matches.

Proof of Code

Since there isn't a way to track MultiSigWallet contract address corresponding to the match, therefore I've added the return statements in the LikeRegistry::likeUser and LikeRegistry::matchRewards functions, basically a kind of mutation testing.

So, the revised LikeRegistry::likeUser and LikeRegistry::matchRewards functions would be:

function likeUser(address liked) external payable returns(address matchToMultiSigWallet) {
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[liked] += 1 ether; // Recommended mitigation from the other HIGH severity vulnerability that I submitted
​
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);
matchToMultiSigWallet = matchRewards(liked, msg.sender);
}
}
​
function matchRewards(address from, address to) internal returns(address) {
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");
​
return address(multiSigWallet);
}

Then, create a test file named as DatingDappTest.t.sol inside the test folder with the following content:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import {Test, console} from "forge-std/Test.sol";
import {LikeRegistry} from "../src/LikeRegistry.sol";
import {SoulboundProfileNFT} from "../src/SoulboundProfileNFT.sol";
import {MultiSigWallet} from "../src/MultiSig.sol";
​
contract DatingDappTest is Test {
SoulboundProfileNFT soulboundNFT;
LikeRegistry likeRegistry;
​
address owner = makeAddr("owner");
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address jack = makeAddr("jack");
​
function setUp() public {
vm.startPrank(owner);
soulboundNFT = new SoulboundProfileNFT();
likeRegistry = new LikeRegistry(address(soulboundNFT));
vm.stopPrank();
}
​
function testUnevenFundsInMultiSigsOnMultipleMatches() public {
address multiSigForMatch_BobAlice;
address multiSigForMatch_JackAlice;
​
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 24, "ipfs://profileImage");
​
vm.prank(jack);
soulboundNFT.mintProfile("Jack", 26, "ipfs://profileImage");
​
hoax(bob, 1 ether);
multiSigForMatch_BobAlice = likeRegistry.likeUser{value: 1 ether}(alice); // Bob liked Alice
​
assertEq(multiSigForMatch_BobAlice, address(0), "There is no match as it's only one-sided like till now");
​
hoax(jack, 1 ether);
multiSigForMatch_JackAlice = likeRegistry.likeUser{value: 1 ether}(alice); // Jack liked Alice
​
assertEq(multiSigForMatch_JackAlice, address(0), "There is no match as it's only one-sided like till now");
​
hoax(alice, 1 ether);
multiSigForMatch_BobAlice = likeRegistry.likeUser{value: 1 ether}(bob); // Alice liked Bob
​
assertNotEq(multiSigForMatch_BobAlice, address(0), "There is a match as it's a mutual like now");
​
hoax(alice, 1 ether);
multiSigForMatch_JackAlice = likeRegistry.likeUser{value: 1 ether}(jack); // Alice liked Jack
​
assertNotEq(multiSigForMatch_JackAlice, address(0), "There is a match as it's a mutual like now");
​
console.log("Bob-Alice MultiSig Contract Balance: ", multiSigForMatch_BobAlice.balance);
console.log("Jack-Alice MultiSig Contract Balance: ", multiSigForMatch_JackAlice.balance);
​
// Ideally, the balance of Bob-Alice MultiSig Contract should be equal to Jack-Alice MultiSig Contract, but that's not the case
assertNotEq(multiSigForMatch_BobAlice.balance, multiSigForMatch_JackAlice.balance);
}
}

On executing forge test --mt testUnevenFundsInMultiSigsOnMultipleMatches -vvv command, the output logs would be:

Logs:
Bob-Alice MultiSig Contract Balance: 2700000000000000000
Jack-Alice MultiSig Contract Balance: 900000000000000000

Recommended Mitigation

In the LikeRegistry::matchRewards function, instead of resetting the LikeRegistry::userBalances mapping values to 0, you can add if-else conditions to decrement the values of userBalances by 1 ether if their value is more than 1 ether, otherwise reset them to 0. And, instead of assigning the matchUserOne and matchUserTwo with the values of userBalances[from] and userBalances[to] respectively, you can assign them as 1 ether.

function matchRewards(address from, address to) internal returns (address) {
- uint256 matchUserOne = userBalances[from];
- uint256 matchUserTwo = userBalances[to];
- userBalances[from] = 0;
- userBalances[to] = 0;
​
+ if (userBalances[from] > 1 ether) {
+ userBalances[from] -= 1 ether;
+ } else {
+ userBalances[from] = 0;
+ }
​
+ if (userBalances[to] > 1 ether) {
+ userBalances[to] -= 1 ether;
+ } else {
+ userBalances[to] = 0;
+ }
​
+ uint256 matchUserOne = 1 ether;
+ uint256 matchUserTwo = 1 ether;
​
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");
​
return address(multiSigWallet);
}
Updates

Appeal created

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_likeUser_no_userBalances_updated

Likelihood: High, always. Impact: High, loss of funds

Support

FAQs

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