DatingDapp

First Flight #33
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Submission Details
Severity: low
Invalid

Users can bypass payments and fees in `LikeRegistry.sol` by creating and funding the wallet themselves

Summary

Upon a match, wallet funds and fees are calculated. The latter can be claimed by the app owner. However, it's also possible to create and fund the MultiSigWallet directly, avoiding paying 1 ETH for a like, as well as fees.

Vulnerability Details

Upon a match between two users, LikeRegistry::matchRewards will get 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;
// 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");
}

matchRewards is calculating the funds for the shared wallet by adding all the previous like payments of the matched users, minus a 10% FIXED_FEE that goes to the app owner via withdrawFees.

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");
}

However, nothing prevents two users from creating and funding a MultiSigWallet themselves,

contract MultiSigWallet {
// ...
modifier onlyOwners() {
if (msg.sender != owner1 && msg.sender != owner2) revert NotAnOwner();
_;
}
constructor(address _owner1, address _owner2) {
require(
_owner1 != address(0) && _owner2 != address(0),
"Invalid owner address"
);
require(_owner1 != _owner2, "Owners must be different");
owner1 = _owner1;
owner2 = _owner2;
}
// ...
/// @notice Allows the contract to receive ETH
receive() external payable {}
}

effectively bypassing any ETH-like payments in LikeRegistry::likeUser, as well as app owner fees.

Proof of Concept

The following code demonstrates the ability of two app users to create a MultiSigWallet, fund it, and perform transactions as they see fit. Place test_UsersCanCreateAndFundWallet in testSoulboundProfileNFT.t.sol:

function test_UsersCanCreateAndFundWallet() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
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");
MultiSigWallet multiSigWallet = new MultiSigWallet(bob, alice);
// bob funds the wallet
vm.prank(bob);
(bool success, ) = payable(address(multiSigWallet)).call{
value: 1 ether
}("");
require(success, "Transaction failed");
// alice funds the wallet
vm.prank(alice);
(success, ) = payable(address(multiSigWallet)).call{value: 1 ether}("");
require(success, "Transaction failed");
assertEq(multiSigWallet.owner1(), bob);
assertEq(multiSigWallet.owner2(), alice);
assertEq(address(multiSigWallet).balance, 2 ether);
vm.startPrank(bob);
address restaurant = makeAddr("restaurant");
// submit a transaction
multiSigWallet.submitTransaction(restaurant, 1 ether);
(address transactionTo, uint256 transactionValue, , , ) = multiSigWallet
.transactions(0);
assertEq(transactionTo, restaurant);
assertEq(transactionValue, 1 ether);
// bob and alice approve the transaction
multiSigWallet.approveTransaction(0);
vm.startPrank(alice);
multiSigWallet.approveTransaction(0);
// execute the transaction
multiSigWallet.executeTransaction(0);
assertEq(address(multiSigWallet).balance, 1 ether);
}

And run the test:

$ forge test --mt test_UsersCanCreateAndFundWallet
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_UsersCanCreateAndFundWallet() (gas: 998243)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.02ms (1.50ms CPU time)
Ran 1 test suite in 147.60ms (8.02ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

This workaround defeats an essential feature of the app.

Tools used

Manual review, tests

Recommendations

Add proper checks in the constructor of MultiSigWallet to make sure that wallet's owners are registered in DatingDapp (minted profile required) and that they like each other.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
+ import "./LikeRegistry.sol";
contract MultiSigWallet {
// ...
modifier onlyOwners() {
if (msg.sender != owner1 && msg.sender != owner2) revert NotAnOwner();
_;
}
+
+ modifier onlyValidOwners(
+ address payable _likeRegistry,
+ address _owner1,
+ address _owner2
+ ) {
+ require(
+ _owner1 != address(0) && _owner2 != address(0),
+ "Invalid owner address"
+ );
+ require(_owner1 != _owner2, "Owners must be different");
+
+ LikeRegistry likeRegistry = LikeRegistry(_likeRegistry);
+ require(
+ likeRegistry.profileNFT().profileToToken(_owner1) != 0,
+ "_owner1 must have a profile NFT"
+ );
+ require(
+ likeRegistry.profileNFT().profileToToken(_owner2) != 0,
+ "_owner2 must have a profile NFT"
+ );
+ require(
+ likeRegistry.likes(_owner1, _owner2),
+ "_owner1 must like each _owner2_"
+ );
+ require(
+ likeRegistry.likes(_owner2, _owner1),
+ "_owner2 must like _owner1"
+ );
+ _;
+ }
+
constructor(
+ address payable _likeRegistry,
address _owner1,
address _owner2
+ ) onlyValidOwners(_likeRegistry, _owner1, _owner2) {
owner1 = _owner1;
owner2 = _owner2;
}
// ...
/// @notice Allows the contract to receive ETH
+ receive() external payable onlyOwners {}
}

Place test_UsersNeedProfileAndToMatchToCreateAWallet in testSoulboundProfileNFT.t.sol and run it with forge test --mt test_UsersNeedProfileAndToMatchToCreateAWallet to verify proper construction of a MultiSigWallet.

function test_UsersNeedProfileAndToMatchToCreateAWallet() public {
vm.deal(bob, 10 ether);
vm.deal(alice, 10 ether);
vm.expectRevert("_owner1 must have a profile NFT");
new MultiSigWallet(payable(address(likeRegistry)), bob, alice);
// mint a profile NFT for bob
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.expectRevert("_owner2 must have a profile NFT");
new MultiSigWallet(payable(address(likeRegistry)), bob, alice);
// mint a profile NFT for alice
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 25, "ipfs://profileImage");
vm.expectRevert("_owner1 must like _owner2");
new MultiSigWallet(payable(address(likeRegistry)), bob, alice);
// bob likes alice
vm.prank(bob);
likeRegistry.likeUser{value: 1 ether}(alice);
assertTrue(likeRegistry.likes(bob, alice));
vm.expectRevert("_owner2 must like _owner1");
new MultiSigWallet(payable(address(likeRegistry)), bob, alice);
// alice likes bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
assertTrue(likeRegistry.likes(alice, bob));
// wallet creation is now successful
new MultiSigWallet(payable(address(likeRegistry)), bob, alice);
}
Updates

Appeal created

n0kto Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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