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 {
require(msg.value >= 1 ether, "Must send at least 1 ETH");
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
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;
uint256 totalRewards = matchUserOne + matchUserTwo;
uint256 matchingFees = (totalRewards * FIXEDFEE) / 100;
uint256 rewards = totalRewards - matchingFees;
totalFees += matchingFees;
MultiSigWallet multiSigWallet = new MultiSigWallet(from, to);
(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:
user1 mints his profile
user2 mints her profile
user1 likes user2
user2 likes user1
matchRewards
is called and both rewards
and totalFees
are zero
multisig wallet has zero funds and app owner can't withdraw fees
ETH is locked in the contract
Code
To demonstrate the aforementioned scenario, apply the following changes to the files below:
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);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
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);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
assertEq(address(likeRegistry).balance, 2 ether);
assertEq(likeRegistry.totalFees(), 0);
vm.stopPrank();
vm.prank(owner);
vm.expectRevert("No fees to withdraw");
likeRegistry.withdrawFees();
vm.stopPrank();
}
function test_executeTransactionFailure() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
vm.stopPrank();
MultiSigWallet multiSigWallet = likeRegistry.multiSigWallet();
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);
}
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);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
assertEq(likeRegistry.userBalances(bob), 1 ether);
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.stopPrank();
}
function test_withdrawFeesFix() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
vm.deal(owner, 0 ether);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
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);
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
vm.stopPrank();
MultiSigWallet multiSigWallet = likeRegistry.multiSigWallet();
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.