code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L80-L86

Vulnerability details

Impact

There is a possible DOS situation, due to an unbounded array. In line https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L80-L86,

for (uint256 i = 0; i < numTargets; ++i) {
                // Execute the transaction
                (bool success, ) = _targets[i].call{ value: _values[i] }(_calldatas[i]);

                // Ensure the transaction succeeded
                if (!success) revert EXECUTION_FAILED(i);
            }

When the size of _targets[] array will increase, the amount of required gas in order to make the external call will also increase. And as the array is unbounded, so the size may increase to an infinite amount. So when the size gets too big, the amount of gas required to make the external call may surpass the block gas limit, which will revert the transaction leading to DOS

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L80-L86

Tools Used

Manual review

Recommended Mitigation Steps

Put an upper bound to the _targets[] array

GalloDaSballo commented 2 years ago

Downgrading to QA R as governance would have to spend X time to vote on something without writing an integration test.