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

`LikeRegistry::userBalances` isn't properly updated, leading to multiple issues and locked funds

stratan

Summary

LikeRegistry::likeUser accepts likes in the form of (at least) 1 ETH. However, these aren't reflected in the code's logic, which can lead to multiple issues, such as a fundless multisig wallet, inability to make transactions with the multisig wallet, an app owner not being able to withdraw fees and effectively locked funds.

Vulnerability Details

LikeRegistry::likeUser is responsible for handling likes between users.

function likeUser(address liked) external payable {
// [1]
require(msg.value >= 1 ether, "Must send at least 1 ETH");
// ...
​
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
​
// [2]
// 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);
}
}

According to the docs and the code above, a user can express interest in someone by paying (at least) 1 ETH ([1]). Should a match happen ([2]), LikeRegistry::matchRewards will be called.

function matchRewards(address from, address to) internal {
uint256 matchUserOne = userBalances[from];
uint256 matchUserTwo = userBalances[to];
userBalances[from] = 0;
userBalances[to] = 0;
​
// [1]
uint256 totalRewards = matchUserOne + matchUserTwo;
// [2]
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);
​
// [3]
// Send ETH to the deployed multisig wallet
(bool success,) = payable(address(multiSigWallet)).call{value: rewards}("");
require(success, "Transfer failed");
}

matchRewards will collect ([1]) all their previous like payments (minus a 10% fee [2]) and finally create a shared multisig wallet between the two users, which they can access for their first date ([3]). Note how userBalances isn't updated in LikeRegistry::likeUser with the incoming ETH-like payments. As the wallet's funds (rewards) and totalFees depend fundamentally on userBalances, they will always be zero. LikeRegistry::withdrawFees will always fail.

function withdrawFees() external onlyOwner {
@> require(totalFees > 0, "No fees to withdraw");
uint256 totalFeesToWithdraw = totalFees;
console.log("totalFeesToWithdraw:", totalFeesToWithdraw);
totalFees = 0;
(bool success, ) = payable(owner()).call{value: totalFeesToWithdraw}(
""
);
require(success, "Transfer failed");
}

Same applies to MultiSigWallet::executeTransaction since the wallet has no funds.

function executeTransaction(uint256 _txId) external onlyOwners {
require(_txId < transactions.length, "Invalid transaction ID");
Transaction storage txn = transactions[_txId];
require(!txn.executed, "Transaction already executed");
require(
txn.approvedByOwner1 && txn.approvedByOwner2,
"Not enough approvals"
);
​
txn.executed = true;
@> (bool success, ) = payable(txn.to).call{value: txn.value}("");
require(success, "Transaction failed");
​
emit TransactionExecuted(_txId, txn.to, txn.value);
}

In conclusion, the shared multisig wallet will have zero funds, the matched users won't be able to spend any money on their date, and the app owner won't be able to withdraw the match's fees. The funds are locked.

Proof of Concept

The following scenario leads to locked ETH funds:

  1. user1 mints his profile

  2. user2 mints her profile

  3. user1 likes user2

  4. user2 likes user1

  5. matchRewards is called and both rewards and totalFees are zero

  6. multisig wallet has zero funds and app owner can't withdraw fees

  7. ETH is locked in the contract

Code

To demonstrate the aforementioned scenario, apply the following changes to the files below:

  • testSoulboundProfileNFT.t.sol

contract SoulboundProfileNFTTest is Test {
SoulboundProfileNFT soulboundNFT;
+ LikeRegistry likeRegistry;
address user = address(0x123);
address user2 = address(0x456);
address owner = address(this); // Test contract acts as the owner
+ address bob = makeAddr("bob");
+ address alice = makeAddr("alice");
​
// ...
function setUp() public {
soulboundNFT = new SoulboundProfileNFT();
+ likeRegistry = new LikeRegistry(address(soulboundNFT));
}
}

Add the tests below too:

function test_MultiSigBalanceAndTotalFeesAreZero() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
​
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
​
// alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
assertEq(address(likeRegistry).balance, 2 ether);
​
MultiSigWallet multiSigWallet = likeRegistry.multiSigWallet();
assertEq(address(multiSigWallet).balance, 0);
}
​
function test_withdrawFeesFailure() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
​
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
​
// alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
​
// assert likeRegistry's balance is 2 ether
assertEq(address(likeRegistry).balance, 2 ether);
// `totalFees` depends on `userBalances`, which is never updated
// `totalFees` will always be zero (bug)
assertEq(likeRegistry.totalFees(), 0);
vm.stopPrank();
​
vm.prank(owner);
// since `totalFees` will always be zero, `withdrawFees` will fail
vm.expectRevert("No fees to withdraw");
likeRegistry.withdrawFees();
vm.stopPrank();
}
​
function test_executeTransactionFailure() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
​
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
​
// alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
​
vm.stopPrank();
​
MultiSigWallet multiSigWallet = likeRegistry.multiSigWallet();
​
// bob and alice went out to a restaurant and the bill is 1 ether
vm.startPrank(bob);
address restaurant = makeAddr("restaurant");
multiSigWallet.submitTransaction(restaurant, 1 ether);
(address transactionTo, uint256 transactionValue, , , ) = multiSigWallet
.transactions(0);
assertEq(transactionTo, restaurant);
assertEq(transactionValue, 1 ether);
​
multiSigWallet.approveTransaction(0);
vm.startPrank(alice);
multiSigWallet.approveTransaction(0);
​
vm.expectRevert("Transaction failed");
multiSigWallet.executeTransaction(0);
}

  • LikeRegistry.sol

The newly created MultiSigWallet is created locally in LikeRegistry::matchRewards. For demonstration purposes, we can make it globally accessible and add a few logs in matchRewards to confirm our hypothesis.

contract LikeRegistry is Ownable {
struct Like {
address liker;
address liked;
uint256 timestamp;
}
​
SoulboundProfileNFT public profileNFT;
+ MultiSigWallet public multiSigWallet;
​
// ...
​
function matchRewards(address from, address to) internal {
uint256 matchUserOne = userBalances[from];
uint256 matchUserTwo = userBalances[to];
+ console.log("[LikeRegistry::matchRewards] matchUserOne:", matchUserOne);
+ console.log("[LikeRegistry::matchRewards] matchUserTwo:", matchUserTwo);
userBalances[from] = 0;
userBalances[to] = 0;
​
uint256 totalRewards = matchUserOne + matchUserTwo;
uint256 matchingFees = (totalRewards * FIXEDFEE) / 100;
uint256 rewards = totalRewards - matchingFees;
totalFees += matchingFees;
+ console.log("[LikeRegistry::matchRewards] totalFees:", totalFees);
​
// Deploy a MultiSig contract for the matched users
+ multiSigWallet = new MultiSigWallet(from, to);
​
// Send ETH to the deployed multisig wallet
(bool success, ) = payable(address(multiSigWallet)).call{
value: rewards
}("");
require(success, "Transfer failed");
}
​
// ...
}

Now run the tests:

$ forge test --mt test_MultiSigBalanceAndTotalFeesAreZero -vvv
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_MultiSigBalanceAndTotalFeesAreZero() (gas: 1044125)
Logs:
[LikeRegistry::matchRewards] matchUserOne: 0
[LikeRegistry::matchRewards] matchUserTwo: 0
[LikeRegistry::matchRewards] totalFees: 0
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.91ms (1.82ms CPU time)
​
Ran 1 test suite in 144.09ms (7.91ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

We can confirm that matchUserOne and matchUserTwo, which correspond to the match's respective likes, are zero despite their like of 1 ETH. Likewise, totalFees is zero and test_withdrawFeesFailure should revert.

$ forge test --mt test_withdrawFeesFailure
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_withdrawFeesFailure() (gas: 1050639)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.54ms (301.33µs CPU time)
​
Ran 1 test suite in 142.59ms (1.54ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Finally, test_executeTransactionFailure confirms that we're unable to make any transactions since the shared multisig wallet has no funds.

$ forge test --mt test_executeTransactionFailure
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_executeTransactionFailure() (gas: 1180268)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.45ms (2.03ms CPU time)
​
Ran 1 test suite in 139.27ms (7.45ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The impact and likelihood of this issue is high. Valuable funds needed for a date are locked. The app owner can't claim her fees either.

Tools used

Manual review, tests

Recommendations

To ensure the date is properfly funded, and the app owner claims her fees, LikeRegistry::likeUser should update userBalances.

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"
);
​
+ userBalances[msg.sender] += msg.value;
​
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
​
// Check if mutual like
if (likes[liked][msg.sender]) {
console.log("[likeUser] mutual like!");
matches[msg.sender].push(liked);
matches[liked].push(msg.sender);
emit Matched(msg.sender, liked);
matchRewards(liked, msg.sender);
}
}

Applying the above patch, we can ensure the functionality now works as intended by adding the following three tests in testSoulBoundProfileNFT.t.sol:

function test_MultiSigBalanceAndTotalFeesAreZeroFix() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
​
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// assert bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
// make sure `userBalances` is properly updated
assertEq(likeRegistry.userBalances(bob), 1 ether);
​
// assert alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
​
// we have 2 ether (bob's and alice's likes) * 10% (FIXED_FEE) -> 0.2
// likeRegistry's balance should be 0.2, which corresponds to the match's fees
// the remaining goes to their multisig for their date -> 1.8 ether
assertEq(address(likeRegistry).balance, 0.2 ether);
assertEq(address(likeRegistry.multiSigWallet()).balance, 1.8 ether);
vm.stopPrank();
}
​
function test_withdrawFeesFix() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
vm.deal(owner, 0 ether);
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// assert bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
​
// assert alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
​
assertEq(address(likeRegistry).balance, 0.2 ether);
assertEq(address(likeRegistry.multiSigWallet()).balance, 1.8 ether);
​
vm.prank(owner);
likeRegistry.withdrawFees();
assertEq(owner.balance, 0.2 ether);
​
vm.stopPrank();
}
​
function test_executeTransactionFailureFix() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
​
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
​
// mint a profile NFT for Alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
​
// bob <3 alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
​
// alice <3 bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
​
vm.stopPrank();
​
MultiSigWallet multiSigWallet = likeRegistry.multiSigWallet();
​
// bob and alice went out to a restaurant and the bill is 1 ether
vm.startPrank(bob);
address restaurant = makeAddr("restaurant");
multiSigWallet.submitTransaction(restaurant, 1 ether);
multiSigWallet.approveTransaction(0);
vm.startPrank(alice);
multiSigWallet.approveTransaction(0);
multiSigWallet.executeTransaction(0);
​
vm.stopPrank();
}

Let's run the tests to verify:

$ forge test --mt test_MultiSigBalanceAndTotalFeesAreZeroFix -vvv
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_MultiSigWalletZeroBalanceFix() (gas: 1073431)
Logs:
[LikeRegistry::matchRewards] matchUserOne: 1000000000000000000
[LikeRegistry::matchRewards] matchUserTwo: 1000000000000000000
[LikeRegistry::matchRewards] totalFees: 200000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.25ms (1.83ms CPU time)
​
Ran 1 test suite in 138.20ms (7.25ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​
​
$ forge test --mt test_withdrawFeesFix
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_withdrawFeesFix() (gas: 1066600)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.64ms (325.13µs CPU time)
​
Ran 1 test suite in 144.17ms (2.64ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​
​
$ forge test --mt test_executeTransactionFailureFix
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_executeTransactionFailureFix() (gas: 1209619)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.49ms (670.25µs CPU time)
​
Ran 1 test suite in 139.32ms (2.49ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

The shared multisig wallet is now properly funded, the matched users can use it to perform transactions for their date and the app owner can withdraw her fees.

Updates

Appeal created

n0kto Lead Judge 4 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.