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

Risk of Fund Theft Through Malicious Contract Redirection

sauronsol

Summary

The MultiSigWallet contract allows any owner to create transactions to arbitrary contract addresses, which could be exploited by a malicious owner to steal funds by redirecting them through a malicious contract.

Vulnerability Details

In MultiSig.sol, the executeTransaction function:

  1. Allows any owner to create and execute transactions to any address

  2. Does not verify if the destination is a contract

  3. Does not implement any verification of the contract's code

  4. Relies solely on the other owner's due diligence

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;
// @audit no verification of destination contract's code
(bool success,) = payable(txn.to).call{value: txn.value}("");
require(success, "Transaction failed");
​
emit TransactionExecuted(_txId, txn.to, txn.value);
}

Impact

  1. Direct Fund Theft : A malicious owner could redirect funds to themselves through a malicious contract

  2. Social Engineering Risk : The other owner might approve transactions without proper verification

  3. No Recovery Mechanism : Once funds are redirected, they cannot be recovered

Severity: MEDIUM - Requires one malicious owner and the other owner's approval, but leads to direct fund theft

Tools Used

  • Manual code review

  • Foundry for testing

  • PoC demonstrating fund theft through contract redirection

Proof of Concept

contract FundStealerContract {
address public maliciousOwner;
constructor(address _maliciousOwner) {
maliciousOwner = _maliciousOwner;
}
receive() external payable {
// Immediately redirect funds to malicious owner
payable(maliciousOwner).transfer(msg.value);
}
}
​
function testMaliciousOwnerFundStealing() public {
// Deploy malicious contract that will redirect funds
FundStealerContract stealer = new FundStealerContract(owner1);
// Initial balance of malicious owner
uint256 initialOwner1Balance = owner1.balance;
// Owner1 (malicious) creates transaction to their contract
vm.prank(owner1);
multiSig.submitTransaction(address(stealer), 5 ether);
// Both owners approve (owner2 doesn't know it's malicious)
vm.prank(owner1);
multiSig.approveTransaction(0);
vm.prank(owner2);
multiSig.approveTransaction(0);
// Execute the transaction
vm.prank(owner1);
multiSig.executeTransaction(0);
// Verify funds were redirected to owner1
assertEq(owner1.balance, initialOwner1Balance + 5 ether, "Funds should be redirected to owner1");
assertEq(address(stealer).balance, 0, "Malicious contract should not keep funds");
}

Recommendations

  1. Implement contract verification and timelock:

mapping(uint256 => uint256) public contractTransactionTimelocks;
​
function submitTransaction(address _to, uint256 _value) external onlyOwners {
// ... existing checks ...
// Check if destination is a contract
uint256 size;
assembly {
size := extcodesize(_to)
}
if (size > 0) {
// Add 24h timelock for contract transactions
uint256 txId = transactions.length;
contractTransactionTimelocks[txId] = block.timestamp + 24 hours;
// Emit event for transparency
emit ContractTransactionSubmitted(txId, _to, _value);
}
transactions.push(Transaction(_to, _value, false, false, false));
}
​
function executeTransaction(uint256 _txId) external onlyOwners {
// ... existing checks ...
// Check timelock for contract transactions
if (contractTransactionTimelocks[_txId] > 0) {
require(
block.timestamp >= contractTransactionTimelocks[_txId],
"Contract transaction is timelocked"
);
}
// ... rest of the function ...
}

  1. Add contract verification requirements:

interface IVerifiableContract {
function verifyContract() external pure returns (bytes32);
}
​
mapping(bytes32 => bool) public approvedContractTypes;
​
function executeTransaction(uint256 _txId) external onlyOwners {
// ... existing checks ...
uint256 size;
assembly {
size := extcodesize(txn.to)
}
if (size > 0) {
try IVerifiableContract(txn.to).verifyContract() returns (bytes32 contractType) {
require(approvedContractTypes[contractType], "Contract type not approved");
} catch {
revert("Contract verification failed");
}
}
// ... rest of the function ...
}

  1. Implement a whitelist for contract destinations:

mapping(address => bool) public whitelistedContracts;
​
function addToWhitelist(address _contract) external {
require(msg.sender == owner1 && msg.sender == owner2, "Both owners must approve");
whitelistedContracts[_contract] = true;
}
​
function executeTransaction(uint256 _txId) external onlyOwners {
// ... existing checks ...
uint256 size;
assembly {
size := extcodesize(txn.to)
}
if (size > 0) {
require(whitelistedContracts[txn.to], "Contract not whitelisted");
}
// ... rest of the function ...
}
Updates

Appeal created

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

Users mistake, only impacting themselves.

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood 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.