hyh - Streaming can be effectively disabled for all the new sales by parameter manipulation

[sherlock-admin4]

hyh

Medium

Streaming can be effectively disabled for all the new sales by parameter manipulation

Summary

Minority protection isn't guaranteed by parameter management logic as _streamLengthInTicks can be set to very low value. E.g. setting it to 1 is allowed, keeps the logic fully operational and causes the whole streaming value to be sent over to treasury on the next tick, i.e. after minimumTickDuration passes once. Setting immediateTreasuryBPs == 10_000 also disables all the new streaming and is allowed

Root Cause

Stream length can be updated, while setting streamLengthInTicks to a very low value essentially negates streaming:

NounsAuctionHouseV3.sol#L261-L271

    function setStreamEscrowParams(
        address _streamEscrow,
        uint16 _immediateTreasuryBPs,
        uint16 _streamLengthInTicks
    ) external onlyOwner {
        streamEscrow = IStreamEscrow(_streamEscrow);
        emit StreamEscrowUpdated(_streamEscrow);

        setImmediateTreasuryBPs(_immediateTreasuryBPs);
>>      setStreamLengthInTicks(_streamLengthInTicks);
    }

NounsAuctionHouseV3.sol#L287-L291

    function setStreamLengthInTicks(uint16 _streamLengthInTicks) public onlyOwner {
>>      require(_streamLengthInTicks > 0, 'streamLengthInTicks too low');
>>      streamLengthInTicks = _streamLengthInTicks;
        emit StreamLengthInTicksUpdated(_streamLengthInTicks);
    }

Also, setting immediateTreasuryBPs to 10_000 or any high enough value nearby is allowed and similarly negates streaming logic:

NounsAuctionHouseV3.sol#L277-L281

    function setImmediateTreasuryBPs(uint16 _immediateTreasuryBPs) public onlyOwner {
>>      require(_immediateTreasuryBPs <= 10_000, 'immediateTreasuryBPs too high');
        immediateTreasuryBPs = _immediateTreasuryBPs;
        emit ImmediateTreasuryBPsUpdated(_immediateTreasuryBPs);
    }

Internal pre-conditions

Rent seeking majority wins over the DAO, passes the proposal to disable streaming via parameters

External pre-conditions

None, new parameters can be set via proposal

Attack Path

DAO sets parameters fully disabling streaming

Impact

All new sold nouns' owners will not have minority protection

PoC

DAO majority passes and executes a proposal fully removing streaming for all the subsequent noun sales by setting streamLengthInTicks low (to 1) or immediateTreasuryBPs high (to 10_000)

Mitigation

As parameters can be reset by a majority vote there is no real minority protection until there a hard coded boundary values.

Consider introducing a material maximum for immediateTreasuryBPs, e.g. 30%:

NounsAuctionHouseV3.sol#L277-L281

    function setImmediateTreasuryBPs(uint16 _immediateTreasuryBPs) public onlyOwner {
-       require(_immediateTreasuryBPs <= 10_000, 'immediateTreasuryBPs too high');
+       require(_immediateTreasuryBPs <= 3_000, 'immediateTreasuryBPs too high');
        immediateTreasuryBPs = _immediateTreasuryBPs;
        emit ImmediateTreasuryBPsUpdated(_immediateTreasuryBPs);
    }

However, due to AuctionHouse and StreamEscrow integration introduced there might be the need to disable StreamEscrow call from AuctionHouse for a while, as described in an another issue, for which either outright switch is needed or 10_000 setting should be allowed particularly for this purpose.

Consider making streamLengthInTicks immutable or introducing a material minimum for it, e.g. 6 months:

NounsAuctionHouseV3.sol#L287-L291

    function setStreamLengthInTicks(uint16 _streamLengthInTicks) public onlyOwner {
-       require(_streamLengthInTicks > 0, 'streamLengthInTicks too low');
+       require(_streamLengthInTicks > 182, 'streamLengthInTicks too low');
        streamLengthInTicks = _streamLengthInTicks;
        emit StreamLengthInTicksUpdated(_streamLengthInTicks);
    }