26017 - [SC - Insight] getTransactionCount will break at some point ru...

Submitted on Nov 23rd 2023 at 02:11:00 UTC by @dontonka for Boost | DeGate

Report ID: #26017

Report type: Smart Contract

Report severity: Insight

Target: https://etherscan.io/address/0x2028834B2c0A36A918c10937EeA71BE4f932da52#code

Impacts:

• Unbounded gas consumption

Description

Bug Description

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.

• git clone https://github.com/immunefi-team/forge-poc-templates.git

• forge init --template immunefi-team/forge-poc-templates --branch default

• Add the following code in src folder, which is the MultiSigWallet.sol contract but upgraded to compile with Solc 0.8.19.

• Replace test/PoCTest.sol with the following test.

• forge test -vv --match-test testAttack

Running 1 test for test/PoCTest.sol:PoCTest
[PASS] testAttack() (gas: 30451777)
Logs:

>>> Initial conditions: # transaction submitted --> 12000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 407.08ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "@immunefi/src/PoC.sol";
import "../src/MultiSigWallet.sol";

contract PoCTest is PoC {
    MultiSigWallet public walletContract;
    address constant ownerAccount1 = 0x2385D7aB31F5a470B1723675846cb074988531da;
    address constant ownerAccount2 = 0x0000000000000000000000000000000000000001;

    function setUp() public {
        deal(ownerAccount1, 1 ether);
        deal(ownerAccount2, 1 ether);

        // Deploy attack contract
        address[] memory accounts = new address[](2);
        accounts[0] = ownerAccount1;
        accounts[1] = ownerAccount2;
        walletContract = new MultiSigWallet(accounts, 2);
        
        // add dummy txs
        bytes memory localBytes = new bytes(1);
        for (uint k=0; k<12000; k++) {
            vm.prank(ownerAccount1);
            walletContract.submitTransaction(accounts[0], 0, localBytes);
        }

        uint txcount = walletContract.transactionCount();
        console.log("\n>>> Initial conditions: # transaction submitted -->", txcount);
    }

    function testAttack() public {
        walletContract.getTransactionCount(true, true);
    }
}

/**
 *Submitted for verification at Etherscan.io on 2021-03-04
*/

/**
 *Submitted for verification at Etherscan.io on 2018-05-10
*/

pragma solidity ^0.8.13;

/// @title Multisignature wallet - Allows multiple parties to agree on transactions before execution.
/// @author Stefan George - <[e-mail]>
contract MultiSigWallet {

    uint constant public MAX_OWNER_COUNT = 50;

    event Confirmation(address indexed sender, uint indexed transactionId);
    event Revocation(address indexed sender, uint indexed transactionId);
    event Submission(uint indexed transactionId);
    event Execution(uint indexed transactionId);
    event ExecutionFailure(uint indexed transactionId);
    event Deposit(address indexed sender, uint value);
    event OwnerAddition(address indexed owner);
    event OwnerRemoval(address indexed owner);
    event RequirementChange(uint required);

    mapping (uint => Transaction) public transactions;
    mapping (uint => mapping (address => bool)) public confirmations;
    mapping (address => bool) public isOwner;
    address[] public owners;
    uint public required;
    uint public transactionCount;

    struct Transaction {