Active Boosts

26116 - [SC - Insight] The MultiSigWalletgetTransactionIds function co...

Submitted on Nov 25th 2023 at 12:28:13 UTC by @OxSCSamurai for Boost | DeGate

Report ID: #26116

Report type: Smart Contract

Report severity: Insight

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

Impacts:

  • Function does not return expected value(s) due to logic bug

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

Description

Bug Description

The MultiSigWallet::getTransactionIds() function contains logic bugs which would exclude one valid transaction ID from the defined range in the returned list.

The getTransactionIds() function is designed to return an array of transaction IDs within a specified range. However, there are logic errors in the function which would result in the function returning a value(s) which is not expected, but the caller of the function will not know any better, and just assume the returned value is correct, or complete, which it isn't.

The first logic error occurs when calculating/assigning the length of the return array _transactionIds. Instead of using ((to + 1) - from) to represent the correct number of entries(and array length), the statement on L373 incorrectly uses (to - from): _transactionIds = new uint[](to - from);

This will result in an incorrect array length every time, and therefore exclude one of the valid entries that was supposed to be returned.

To demonstrate the logic bug/error: Lets say from = 0 and to = 5, then the correct number of entries should be 6. But why you ask? Here you go: 0,1,2,3,4,5 = SIX entries. Therefore the array length should be 6. However, (to - from) = (5 - 0) = 5, which is 1 less than 6.

The second logic error is here on L374: for (i=from; i<to; i++) It excludes a valid index/entry, which is represented by the value of to. Yes, usually we do i < array.length, so what is the correct length in this example? Remember, it is 6. So if we do i<6 then it would be correct, but the current implementation uses i<to which is i<5, which is incorrect, and will exclude the final entry, the 6th entry from the returned results.

One last time: 0,1,2,3,4,5 represents 6 entries, not 5, even though 5-0=5 OR 1,2,3,4,5,6 represents 6 entries, not 5, even though 6-1=5 OR 2,3,4,5,6,7 represents 6 entries, not 5, even though 7-2=5

Here's the function as its currently implemented:

    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];
    }

Impact

  • local impact on the function's returned result is that it will ALWAYS exclude one valid entry, the last entry, represented by to in the defined range.

  • currently not sure of impact on protocol/users, as this is a constant/view only function it seems, so it only returns a value, I will need to check with the devs if there is any significant protocol dependencies that depend on the return value of this function, and if there are indeed critical dependencies, we might have to upgrade the severity level to high, depending.

Risk Breakdown

Difficulty to Exploit: Easy Weakness: CVSS2 Score:

Recommendation

    /// @dev Returns list of transaction IDs in defined range. /// @audit the defined range is from `from` to `to`
    /// @param from Index start position of transaction array. /// @audit for case example lets say `from` is 0
    /// @param to Index end position of transaction array. /// @audit for case example lets say `to` is 5
    /// @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);
+       _transactionIds = new uint[]((to + 1) - from);
-       for (i=from; i<to; i++)
+       for (i=from; i<=to; i++) /// @audit OR alternatively: `for (i=from; i<to+1; i++)`
            _transactionIds[i - from] = transactionIdsTemp[i];
    }

References

Proof of concept

(Please note: My experience with foundry tests is new, so please make sure you understand my tests correctly, especially how I constructed them. I added comments in the test contract in the test functions to help you to understand my logic and method/approach. Please make sure to read those comments to avoid any confusion. Feel free to give me a shout if you need clarifications/explanations.)

I added the fixed function to the multisig wallet contract temporarily so that I could run my PoC tests to test my fix too, not just the logic bug.

Here it is, and then below the fixed function is my test contract, I used foundry, with some magic...

    /// audit added this function temporarily for PoC testing purposes only.
    function fixed_getTransactionIds(uint from, uint to, bool pending, bool executed)
        public
        view
        returns (uint[] memory _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 + 1) - from);
       for (i=from; i<=to; i++) /// OR alternatively: `for (i=from; i<to+1; i++)`
            _transactionIds[i - from] = transactionIdsTemp[i];
    }

TEST CONTRACT:

pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import {MultiSigWallet} from "src/MultiSigWallet.sol";

contract TestMultiSigWallet is Test {

    MultiSigWallet multiSigWallet;

    function setUp() public {
        /// prepping the parameter values for the constructor of the MultiSigWallet. making the deployer address an owner, because why not, for this test should be valid.
        address[] memory _owners = new address[](1);
        uint _required;
        (_owners[0], _required) = (0xb4c79daB8f259C7Aee6E5b2Aa729821864227e84, 1