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

Add ReentrancyGuard in `MultiSigWallet` to prevent unforeseen reentrancy paths in future modifications

stratan

Summary

Even though MultiSigWallet::executeTransaction follows the check-effects-interactions pattern correctly, future modifications in the function could introduce reentrancy paths.

Details

The function MultiSigWallet::executeTransaction follows the check-effects-interactions pattern correctly.

function executeTransaction(uint256 _txId) external onlyOwners {
// Checks
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"
);
​
// Effects
txn.executed = true;
​
// Interactions
(bool success, ) = payable(txn.to).call{value: txn.value}("");
require(success, "Transaction failed");
​
emit TransactionExecuted(_txId, txn.to, txn.value);
}

However, should there be future modifications in the function, it could introduce reentrancy issues.

Tools Used

Manual review

Recommendations

While the current pattern is safe, you can add an extra layer of security by using OpenZeppelin's ReentrancyGuard:

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
+ contract MultiSigWallet is Ownable, ReentrancyGuard {
​
// ...
​
+ function executeTransaction(uint256 _txId) external onlyOwners nonReentrant {
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;
(bool success, ) = payable(txn.to).call{value: txn.value}("");
require(success, "Transaction failed");
​
emit TransactionExecuted(_txId, txn.to, txn.value);
}
}
Updates

Appeal created

n0kto Lead Judge 6 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.