Summary

There is a reentrancy vulnerability in the MultiSig:executeTransaction function,

Here is the link that shows the part of code in which has the vulnerability.

It does an external call to transfer some ETH before updating any state variables, in which if the recipient contract that is being called has an fallback or receive` functions that can call back the MultiSigWallet contract, it could reenter the MultiSig:executeTransaction function and keep draining funds until the contract is fully drained.

Vulnerability Details

This type of reentrancy attack is really common, it occurs when a attacker or a malicious party contract calls a specific function in the contract, exploiting a vulnerability in the contract, If the malicious party contract has fallback or receive` functions, it can lead to repeated calls to this function to keep draining money off the contract to benefit themselves.

This mainly happens because there isn't any change in the state variables before doing the external call, only after.

Proof of Concept

1. Attacker calls the MultiSig:executeTransactionby a contract, sends Eth to the contract

2. Attacker has an receivefunction

3. Attacker keeps reentering this function by calling the functions submit, approve and execute` and draining money from the contract

Here is a simple attacker contract that proves you can do this:

Impact

Attacker can keep reentering the contract and calling the executeTransaction` function over and over again and drain all the money from the protocol.

Tools Used

Manual Review

Slither and Aderyn

Recommendations

Since theexecuteTransactionapparently doesn't follow CEI(Checks, Effects and Interactions), I sugest to follow it and change the function a bit to update state variables before doing any external calls, since this is the root cause of the reentrancy problem.