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

Potential Reentrancy Risk in MultiSigWallet's executeTransaction

sauronsol

Summary

The MultiSigWallet contract uses a low-level call to transfer ETH in the executeTransaction function. Although protected by the executed flag, this approach is not optimal in terms of security.

Vulnerability Details

In MultiSig.sol, the executeTransaction function:

  1. Uses a low-level call to transfer ETH

  2. Relies solely on the executed flag to prevent reentrancy

  3. Does not implement an explicit ReentrancyGuard

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 low level call of funds
(bool success,) = payable(txn.to).call{value: txn.value}("");
require(success, "Transaction failed");
​
emit TransactionExecuted(_txId, txn.to, txn.value);
}

Impact

  1. Implicit Protection : The protection against reentrancy relies on business logic rather than a dedicated mechanism

  2. Future Risk : The addition of new features could introduce vulnerabilities if the pattern is not maintained

  3. Non-Compliance : Does not follow recommended security best practices

Severity: LOW - The vulnerability is mitigated by the onlyOwners modifier and the executed flag

Tools Used

  • Foundry for testing

  • Manual code review

  • PoC demonstrating the reentrancy attempt

Proof of Concept

contract AttackerContract {
MultiSigWallet public multiSig;
uint256 public attackCount;
uint256 public txId;
​
constructor(address _multiSig) {
multiSig = MultiSigWallet(payable(_multiSig));
}
​
receive() external payable {
if (attackCount < 3) {
attackCount++;
multiSig.executeTransaction(txId);
}
}
}
​
function testReentrancyAttack() public {
// The reentrancy attempt fails due to the executed flag
vm.expectRevert("Transaction already executed");
multiSig.executeTransaction(txId);
}

Recommendations

  1. Implement OpenZeppelin's ReentrancyGuard:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract MultiSigWallet is ReentrancyGuard {
function executeTransaction(uint256 _txId) external nonReentrant onlyOwners {
// ... rest of the code ...
}
}

  1. Use a more explicit security pattern:

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");
​
// Save the values
address to = txn.to;
uint256 value = txn.value;
// Update the state before external interaction
txn.executed = true;
emit TransactionExecuted(_txId, to, value);
​
// Perform the transfer last
(bool success,) = payable(to).call{value: value}("");
require(success, "Transfer failed");
}

  1. Consider using a pull-over-push pattern:

mapping(address => uint256) public pendingWithdrawals;
​
function executeTransaction(uint256 _txId) external onlyOwners {
// ... verifications ...
txn.executed = true;
pendingWithdrawals[txn.to] += txn.value;
emit TransactionExecuted(_txId, txn.to, txn.value);
}
​
function withdraw() external {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "No funds to withdraw");
pendingWithdrawals[msg.sender] = 0;
(bool success,) = payable(msg.sender).call{value: amount}("");
require(success, "Transfer failed");
}
Updates

Appeal created

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

invalid_reentrancy_with_no_impact

matchRewards: Contract is created just before and is the one called. No impact. executeTransaction: CEI is followed. Emitting an event in disorder is informational in that context. withdraw: CEI is followed.

Support

FAQs

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