Description
The MultiSigWallet::submitTransaction function doesn't have any check for the maximum amount (_value) that an owner can withdraw from the contract, and also there is no limit on the number of transactions that an owner can submit. Although, the transaction can't be executed until the MultiSigWallet::approveTransaction function is being called by both the owners, so the owner can optionally decide not to approve the transaction submitted by the other owner, but it would still impose the problem of either a lot of unapproved transactions in the queue (i.e., transactions array) or the owner accidentally approving the transaction without inspecting the values of the underlying fields manually.
Impact
Either of the two owners can drain the MultiSigWallet contract, causing the other owner losing their funds.
Proof of Concept
Once a match is being made and the MultiSigWallet contract is deployed and funded with ether.
Either of the two owners (i.e., users corresponding to the match) of the MultiSigWallet contract can call MultiSigWallet::submitTransaction function any number of times and by passing any _value greater than zero.
If the other owner accidentally calls the MultiSigWallet::approveTransaction function without inspecting the values of the underlying fields manually, then it could result in the draining of funds from the MultiSigWallet contract.
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;
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);
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;
MultiSigWallet multiSigWallet = new MultiSigWallet(from, to);
(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:
pragma solidity 0.8.20;
import {Test} 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");
function setUp() public {
vm.startPrank(owner);
soulboundNFT = new SoulboundProfileNFT();
likeRegistry = new LikeRegistry(address(soulboundNFT));
vm.stopPrank();
}
function testEitherOwnerCanDrainMultiSig() public {
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://profileImage");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 24, "ipfs://profileImage");
hoax(bob, 1 ether);
address multiSigForMatch = likeRegistry.likeUser{value: 1 ether}(alice);
assertEq(multiSigForMatch, address(0), "There is no match because it's only one-sided like till now");
hoax(alice, 1 ether);
multiSigForMatch = likeRegistry.likeUser{value: 1 ether}(bob);
assertNotEq(multiSigForMatch, address(0), "There is a match because it's a mutual like now");
uint256 totalAmountInMultiSigWallet = multiSigForMatch.balance;
assertEq(totalAmountInMultiSigWallet, 1.8 ether);
MultiSigWallet multiSigWallet = MultiSigWallet(payable(multiSigForMatch));
vm.startPrank(bob);
multiSigWallet.submitTransaction(bob, totalAmountInMultiSigWallet);
multiSigWallet.approveTransaction(0);
vm.stopPrank();
vm.prank(alice);
multiSigWallet.approveTransaction(0);
vm.prank(bob);
multiSigWallet.executeTransaction(0);
assertEq(multiSigForMatch.balance, 0);
assertEq(bob.balance, totalAmountInMultiSigWallet);
}
}
Recommended Mitigation
Add the following checks in the MultiSigWallet::submitTransaction function:
+ error ValueExceedsTheMaxAllowedAmount(uint256 maxAllowedAmount);
+ error Owner1HasAlreadySubmittedTransaction();
+ error Owner2HasAlreadySubmittedTransaction();
/// @notice Submit a transaction for approval
function submitTransaction(address _to, uint256 _value) external onlyOwners {
if (_to == address(0)) revert InvalidRecipient();
if (_value == 0) revert InvalidAmount();
+ if (_value > address(this).balance / 2) revert ValueExceedsTheMaxAllowedAmount(address(this).balance / 2);
+ if (submittedTransactionByOwner1) revert Owner1HasAlreadySubmittedTransaction();
+ if (submittedTransactionByOwner2) revert Owner2HasAlreadySubmittedTransaction();
+ if (msg.sender == owner1) {
+ submittedTransactionByOwner1 = true;
+ } else {
+ submittedTransactionByOwner2 = true;
+ }
transactions.push(Transaction(_to, _value, false, false, false));
uint256 txId = transactions.length - 1;
emit TransactionCreated(txId, _to, _value);
}
