Submission Details
Severity:
Lack of `msg.value` handling in `LikeRegistry::likeUser` locks all the money in the contract.
**Description:** The `likeUser` function in `LikeRegistry` does not handle the money sent by the user when liking another user, as it does not update the `userBalances` variable. This prevents money from later being sent to the users multisig address in `LikeRegistry::matchRewards`, as well as from being withdrawn by the contract owner in `LikeRegistry::withdrawFees`.
```javascript
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);
@> // LACKS UPDATE OF userBalances
// Check if mutual like
if (likes[liked][msg.sender]) {
matches[msg.sender].push(liked);
matches[liked].push(msg.sender);
// q : not emitting event for match the other way around
emit Matched(msg.sender, liked);
matchRewards(liked, msg.sender);
}
}
```
**Impact:** No money can be withdrawn from the contract : neither by any multisig wallet, neither by the contract owner.
**Proof of Concept:**
The `LikeRegistry::userBalances` mapping is never updated when users send funds in `LikeRegistry::likeUser`.
Hence, there is no way for funds to be dispatched correcly in `LikeRegistry::matchRewards` as it relies only on `LikeRegistry::userBalances`, which - again - always returns 0.
```javascript
function matchRewards(address from, address to) internal {
@> uint256 matchUserOne = userBalances[from];
@> uint256 matchUserTwo = userBalances[to];
userBalances[from] = 0;
userBalances[to] = 0;
...
}
```
<details>
<summary>Proof of code</summary>
Add the following code at the end of the testing contract `testSoulboundProfileNFT.t.sol::SoulboundProfileNFTTest` :
```javascript
function testNoMoneyCanBeWithdrawn() public {
LikeRegistry likeRegistry;
MultiSigWallet multiSigWallet;
// assert balance of likeRegistry
assertEq(address(likeRegistry).balance, 0);
// mint profiles
// the first one
address firstLover = makeAddr("firstLover");
vm.prank(firstLover);
soulboundNFT.mintProfile("Alice", 75, "ipfs://aliceProfileImage");
// the second one
address secondLover = makeAddr("secondLover");
vm.prank(secondLover);
soulboundNFT.mintProfile("Bob", 25, "ipfs://bobProfileImage");
// make them like each other
likeRegistry = new LikeRegistry(address(soulboundNFT));
// the first one
vm.deal(firstLover, 1 ether);
vm.prank(firstLover);
likeRegistry.likeUser{value: 1 ether}(secondLover);
// the second one
vm.deal(secondLover, 1 ether);
vm.prank(secondLover);
likeRegistry.likeUser{value: 1 ether}(firstLover);
// assert balance of likeRegistry is 1 + 1 = 2 ether
assertEq(address(likeRegistry).balance, 2 ether);
// assert that owner reverts on "No fees to withdraw"
vm.prank(owner);
vm.expectRevert("No fees to withdraw");
likeRegistry.withdrawFees();
// assert that multisig is empty
multiSigWallet = MultiSigWallet(
payable(address(0xffD4505B3452Dc22f8473616d50503bA9E1710Ac))
);
assertEq(address(multiSigWallet).balance, 0);
}
```
</details>
**Recommended Mitigation:**
- At the very least, do the following :
```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;
emit Liked(msg.sender, liked);
+ userBalances[msg.sender] += msg.value;
// Check if mutual like
if (likes[liked][msg.sender]) {
matches[msg.sender].push(liked);
matches[liked].push(msg.sender);
// q : not emitting event for match the other way around
emit Matched(msg.sender, liked);
matchRewards(liked, msg.sender);
}
}
```
However, simply incrementing the `userBalances` would lead to unfair transfer in case of multiple matches. A better solution would thus be to:
- Add a mapping to store how much a user sent to like a specific profile in `LikeRegistry::likeUser`;
- Use the new mapping in `LikeRegistry::matchRewards` when there is match instead of full balance
```diff
contract LikeRegistry is Ownable {
...
+ mapping(address => mapping(address => uint256)) public balanceToMatch;
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);
+ balanceToMatch[msg.sender][liked] += msg.value;
// Check if mutual like
if (likes[liked][msg.sender]) {
matches[msg.sender].push(liked);
matches[liked].push(msg.sender);
// q : not emitting event for match the other way around
emit Matched(msg.sender, liked);
matchRewards(liked, msg.sender);
}
}
function matchRewards(address from, address to) internal {
- uint256 matchUserOne = userBalances[from];
+ uint256 matchUserOne = balanceToMatch[from][to];
- uint256 matchUserTwo = userBalances[to];
+ uint256 matchUserTwo = balanceToMatch[to][from];
- userBalances[from] = 0;
+ balanceToMatch[from][to] = 0;
- userBalances[to] = 0;
+ balanceToMatch[to][from] = 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");
}
...
}
```
<b>--> A better explanation of why we need a mapping of a mapping instead of just `userBalances`</b>
Some assumptions:
- There are 3 lovers: Alice, Bob and Max.
- Alice first like Bob.
- Alice then like Max, before Max or Bob like her back.
- Bob then like Alice, producing the first match.
- Max then like Alice, producing the second match.
Once a first match has been done for the first two users (Alice and Bob), any other pending like made by any of those users (Alice) will not have the money of Alice anymore, leaving no extra money available for a potential other match. By extension, this also allows a potential attacker to get back 90% of his contribution before it goes to the multisig wallet.
<details>
<summary>Proof of code</summary>
```javascript
function testSecondLikeHasLowerBalanceLeft() public {
LikeRegistry likeRegistry;
MultiSigWallet multiSigWallet1and2;
MultiSigWallet multiSigWallet1and3;
// the first one
address firstLover = makeAddr("firstLover");
vm.prank(firstLover);
soulboundNFT.mintProfile("Alice", 75, "ipfs://aliceProfileImage");
// the second one
address secondLover = makeAddr("secondLover");
vm.prank(secondLover);
soulboundNFT.mintProfile("Bob", 25, "ipfs://bobProfileImage");
address thirdLover = makeAddr("thirdLover");
vm.prank(thirdLover);
soulboundNFT.mintProfile("Max", 22, "ipfs://juliaProfileImage");
// make them like each other
likeRegistry = new LikeRegistry(address(soulboundNFT));
// the first one likes 2 users spending 2 ether
vm.deal(firstLover, 2 ether);
vm.startPrank(firstLover);
likeRegistry.likeUser{value: 1 ether}(secondLover);
likeRegistry.likeUser{value: 1 ether}(thirdLover);
vm.stopPrank();
// the second one likes the first one spending 1 ether
vm.deal(secondLover, 1 ether);
vm.prank(secondLover);
likeRegistry.likeUser{value: 1 ether}(firstLover);
// the third one likes the first one spending 1 ether
vm.deal(thirdLover, 1 ether);
vm.prank(thirdLover);
likeRegistry.likeUser{value: 1 ether}(firstLover);
// assert that multisig with lover1 and lover2 has 3 ether - fee
// multisig address comes from `forge test` logs
multiSigWallet1and2 = MultiSigWallet(
payable(address(0xffD4505B3452Dc22f8473616d50503bA9E1710Ac))
);
assertEq(address(multiSigWallet1and2).balance, 2.7 ether);
// assert that multisig with lover1 and lover3 has only 1 ether - fee
// multisig address comes from `forge test` logs
multiSigWallet1and3 = MultiSigWallet(
payable(address(0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81))
);
assertEq(address(multiSigWallet1and3).balance, 0.9 ether);
}
```
</details>
By using a mapping of a mapping, we segregate the likes and there is no inbalance anymore.
Updates
Appeal created
n0kto 5 months ago
Submission Judgement Published Assigned finding tags:
finding_likeUser_no_userBalances_updated
Likelihood: High, always. Impact: High, loss of funds
Rewards breakdown
nSLOC
197This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.