Summary
MultiSigWallet::receive allows the contract to receive ETH from arbitrary users/addresses. Whoever is not the owner of the wallet can have his ETH locked or stolen should he decide to fund the contract.
Vulnerability Details
Based on the code, MultiSigWallet::receive allows the contract to receive ETH by any user (inside or outside the app).
receive() external payable {}
However, should an app user (or any contract address in fact) were to fund the contract, the ETH would be locked as there's no way to withdraw funds from the wallet, nor perform any transaction except for the wallet's owners.
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;
}
function submitTransaction(
address _to,
uint256 _value
) external onlyOwners {
}
function approveTransaction(uint256 _txId) external onlyOwners {
}
function executeTransaction(uint256 _txId) external onlyOwners {
}
Additionally, the owners could decide to split the extra ETH between them, effectively stealing the funds.
Proof of Concept
We can verify both scenarios. First, add test_NonOwnerCanFundWallet in testSoulboundProfileNFT.t.sol:
function test_NonOwnerCanFundWallet() public {
uint256 WALLET_BALANCE = 10 ether;
MultiSigWallet multiSigWallet = new MultiSigWallet(bob, alice);
vm.deal(address(multiSigWallet), WALLET_BALANCE);
assertEq(multiSigWallet.owner1(), bob);
assertEq(multiSigWallet.owner2(), alice);
address testUser = makeAddr("testUser");
vm.deal(testUser, 5 ether);
vm.prank(testUser);
(bool success, ) = payable(address(multiSigWallet)).call{
value: 1 ether
}("");
require(success, "Transfer failed");
assertEq(address(multiSigWallet).balance, 11 ether);
}
and run the test:
$ forge test --mt test_NonOwnerCanFundWallet
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_NonOwnerCanFundWalletFix() (gas: 533867)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.19ms (336.79µs CPU time)
Ran 1 test suite in 137.48ms (2.19ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Next, we can demonstrate the wallet's owners can steal a user's funds:
Create a wallet with bob and alice as owners
Fund the wallet
bob and alice spend wallet's funds on their date
victim funds the wallet contract
bob and alice steal victim's funds and split it 50/50
Place test_stealNonOwnerFunds in testSoulboundProfileNFT.t.sol:
function test_stealNonOwnerFunds() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address victim = makeAddr("victim");
vm.deal(bob, 5 ether);
vm.deal(alice, 5 ether);
vm.deal(victim, 5 ether);
uint256 WALLET_BALANCE = 10 ether;
uint256 RESTAURANT_BILL = 10 ether;
MultiSigWallet multiSigWallet = new MultiSigWallet(bob, alice);
vm.deal(address(multiSigWallet), WALLET_BALANCE);
vm.startPrank(bob);
address restaurant = makeAddr("restaurant");
multiSigWallet.submitTransaction(restaurant, RESTAURANT_BILL);
multiSigWallet.approveTransaction(0);
vm.startPrank(alice);
multiSigWallet.approveTransaction(0);
multiSigWallet.executeTransaction(0);
assertEq(address(multiSigWallet).balance, 0);
vm.startPrank(victim);
(bool success, ) = payable(address(multiSigWallet)).call{
value: 2 ether
}("");
require(success, "Transfer failed");
assertEq(address(multiSigWallet).balance, 2 ether);
vm.startPrank(bob);
multiSigWallet.submitTransaction(bob, 1 ether);
multiSigWallet.approveTransaction(1);
vm.startPrank(alice);
multiSigWallet.approveTransaction(1);
multiSigWallet.executeTransaction(1);
assertEq(address(bob).balance, 1 ether);
vm.startPrank(alice);
multiSigWallet.submitTransaction(alice, 1 ether);
multiSigWallet.approveTransaction(2);
vm.startPrank(bob);
multiSigWallet.approveTransaction(2);
multiSigWallet.executeTransaction(2);
assertEq(address(alice).balance, 1 ether);
assertEq(address(multiSigWallet).balance, 0);
vm.stopPrank();
}
and run the test:
$ forge test --mt test_stealNonOwnerFunds
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] test_NonOwnerCanFundWallet() (gas: 536625)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.21ms (1.40ms CPU time)
Ran 1 test suite in 141.86ms (7.21ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
ETH can get locked or stolen
Tools Used
Manual review, tests
Recommendations
If the option to fund the MultiSigWallet contract is intended, add the onlyOwners modifer to prevent non-owners from sending ETH.
+ receive() external payable onlyOwners {}
The test below should revert.
function test_NonOwnerCanFundWalletFix() public {
MultiSigWallet multiSigWallet = new MultiSigWallet(bob, alice);
vm.deal(address(multiSigWallet), 10 ether);
assertEq(address(multiSigWallet).balance, 10 ether);
address testUser = makeAddr("testUser");
vm.deal(testUser, 5 ether);
vm.prank(testUser);
vm.expectRevert();
(bool success, ) = payable(address(multiSigWallet)).call{
value: 1 ether
}("");
require(success, "Transfer failed");
}
Alternatively, remove receive completely.
