getTransactionCount vulnerable to run out of gas and DoS even if it's a view function. So whatever clients rely on this view function would not be able todo his job anymore as calling this function would always revert.
Impact
getTransactionCount will always revert once ~1300 transactions (in Remix the gas limit seems to be 3M) have been submitted throught the wallet. The gas limit seems to be 30M thought, I'm not sure why in Remix it revert at 3M, but if it needs to reach 30M, then the cap is around 12000 transactions (see PoC)
Recommendation
The problem is the for loop for (uint i=0; i<transactionCount; i++) which will iterate over all the txs submitted. Even if it's only to update a counter, it will exhaust all the gas available at some point, and this seems to be around having submitted 1300 transactions.
Similar to what getTransactionIds is doing, you would need to introduce a range of ids you want (uint from, uint to), and ensure the for loop is only considering this range.
/// @dev Returns total number of transactions after filers are applied.
/// @param pending Include pending transactions.
/// @param executed Include executed transactions.
/// @return Total number of transactions after filters are applied.
function getTransactionCount(bool pending, bool executed)
public
constant
returns (uint count)
{
for (uint i=0; i<transactionCount; i++)
if ( pending && !transactions[i].executed
|| executed && transactions[i].executed)
count += 1;
}
Proof of concept
You will have to clone the immunefi poc repo and follow the steps below. I had to upgrade the MultiSigWallet code to match solc v0.8.19 compiler in order todo this PoC. Nevertheles, I also tested in Remix using the same compiler (v0.4.26, also using the original code) and the it start reverting at around 1300 txs. Once you run the test, you can see the gas used which is above 30M.
/** *Submitted for verification at Etherscan.io on 2021-03-04*//** *Submitted for verification at Etherscan.io on 2018-05-10*/pragmasolidity ^0.8.13;/// @title Multisignature wallet - Allows multiple parties to agree on transactions before execution./// @author Stefan George - <stefan.george@consensys.net>contract MultiSigWallet {uintconstantpublic MAX_OWNER_COUNT =50;eventConfirmation(addressindexed sender, uintindexed transactionId);eventRevocation(addressindexed sender, uintindexed transactionId);eventSubmission(uintindexed transactionId);eventExecution(uintindexed transactionId);eventExecutionFailure(uintindexed transactionId);eventDeposit(addressindexed sender, uint value);eventOwnerAddition(addressindexed owner);eventOwnerRemoval(addressindexed owner);eventRequirementChange(uint required);mapping (uint=> Transaction) public transactions;mapping (uint=>mapping (address=>bool)) public confirmations;mapping (address=>bool) public isOwner;address[] public owners;uintpublic required;uintpublic transactionCount;structTransaction {