26012 - [SC - Insight] getTransactionIds will break at some point runn...

Submitted on Nov 22nd 2023 at 23:41:52 UTC by @dontonka for Boost | DeGate

Report ID: #26012

Report type: Smart Contract

Report severity: Insight

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

Impacts:

• Unbounded gas consumption

• Contract fails to deliver promised returns, but doesn't lose value

Description

Bug Description

getTransactionIds 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

getTransactionIds will always revert once ~1200 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 11400 transactions (see PoC)

Recommendation

The problem is really the first for loop for (i=0; i<transactionCount; i++) which will always iterate over all the txs submitted. Consider refactoring the implementation such that this for loop is not affected by the transactionCount, but only the Ids range received in parameters. I'm happy to code it if this issue is considered valid.

    /// @dev Returns list of transaction IDs in defined range.
    /// @param from Index start position of transaction array.
    /// @param to Index end position of transaction array.
    /// @param pending Include pending transactions.
    /// @param executed Include executed transactions.
    /// @return Returns array of transaction IDs.
    function getTransactionIds(uint from, uint to, bool pending, bool executed)
        public
        constant
        returns (uint[] _transactionIds)
    {
        uint[] memory transactionIdsTemp = new uint[](transactionCount);
        uint count = 0;
        uint i;
        for (i=0; i<transactionCount; i++)
            if (   pending && !transactions[i].executed
                || executed && transactions[i].executed)
            {
                transactionIdsTemp[count] = i;
                count += 1;
            }
        _transactionIds = new uint[](to - from);
        for (i=from; i<to; i++)
            _transactionIds[i - from] = transactionIdsTemp[i];
    }

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 1200 txs. Once you run the test, you can see the gas used which is above 30M, which confirm the problem.

• 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: 31621915)
Logs:

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

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

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.getTransactionIds(0, 20, 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