Nouns Builder contest - yixxas's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L596-L602

Vulnerability details

Impact

updateVetoer() is extremely important to the security of this contract as the vetoer holds the power to stop any proposal from being executed. If vetoer is set wrongly by accident, to one without an owner, it can open up the protocol to governance attack as described by Dialectic here. If vetoer is set wrongly to a malicious actor, all funds in the protocol will be stuck in contract as all proposals can be vetoed.

Proof of Concept

updateVetoer() does not require the target address to accept this transfer of vetoer privilege.

Governor.sol#L596-L602

    function updateVetoer(address _newVetoer) external onlyOwner {
        if (_newVetoer == address(0)) revert ADDRESS_ZERO();

        emit VetoerUpdated(settings.vetoer, _newVetoer);

        settings.vetoer = _newVetoer;
    }

Recommended Mitigation Steps

Consider adding an acceptVetoer() function as commonly done for critical operations such as transfer of ownership or admin privilege.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L141-L172

Vulnerability details

Impact

propose() function in Governor.sol takes in input from user of unbounded _targets[] array. This can lead to too high a gas consumption required to execute the entire proposal, one that exceeds the block gas limit which prevents anyone from executing this proposal even though it has sufficient for votes casted.

Proof of Concept

1. Proposal is submitted via the _propose() function.
2. Votes are then casted by users.
3. If sufficient users votes in favour of the proposal, execute() can be called.

Treasury.sol#L141-L172

    function execute(
        address[] calldata _targets,
        uint256[] calldata _values,
        bytes[] calldata _calldatas,
        bytes32 _descriptionHash
    ) external payable onlyOwner {
        ...
        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Cannot realistically overflow
        unchecked {
            // For each target:
            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);
            }
        }
        ...
    }

However, since numTargets == _targets.length is unbounded, a proposal that is made with too many targets can lead to gas consumptions which exceeds the block gas limit, causing a proposal that has passed the voting phase to be unable to be executed.

Recommended Mitigation Steps

Set a hard limit on _targets.length as done in nounsDAO. ( max number of targets is set to 10 there )