hyh - Stream cancellation functionality for all old nouns can be disabled via proposal

[sherlock-admin3]

hyh

High

Stream cancellation functionality for all old nouns can be disabled via proposal

Summary

Majority can push a proposal that disables the the ability to cancel a stream via setting nounsRecipient to zero address. This way any minority nouns holder with active stream will be unable to quit and majority will have the control over all the funds being streamed in the future. This affects all current holders with streams, i.e. includes ones previously sold with the premise of immutable minority protection enabled, which is the substantial part of DAO share value

Root Cause

Stream cancellation is a base lever for minority holders to quit when their positions are anyhow threatened. Stream cancellation for all current streams (all the nouns sold after streaming was introduced) can be disabled by setting nounsRecipient to zero address via the corresponding proposal, which is allowed by setNounsRecipient():

StreamEscrow.sol#L278-L284

    /**
     * @notice Allows the DAO to set the address that the Nouns tokens will be sent to when streams are canceled.
     */
    function setNounsRecipient(address newAddress) external onlyDAO {
>>      nounsRecipient = newAddress;
        emit NounsRecipientSet(newAddress);
    }

This will effectively block cancelStream():

StreamEscrow.sol#L161-L171

    /**
     * @notice Cancels a stream for a specific Noun token. Transfers the Noun to `nounRecipient`
     *  and refunds the remaining ETH back to the caller.
     * @notice The caller must be the Noun owner.
     * @param nounId The ID of the Noun token to cancel the stream for.
     */
    function cancelStream(uint256 nounId) public {
        require(isStreamActive(nounId), 'stream not active');

        // transfer noun to treasury
>>      nounsToken.transferFrom(msg.sender, nounsRecipient, nounId);

As transferFrom() -> _transfer() will be reverting on zero recipient:

ERC721.sol#L373-L379

    function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal virtual {
        require(ERC721.ownerOf(tokenId) == from, 'ERC721: transfer of token that is not own');
>>      require(to != address(0), 'ERC721: transfer to the zero address');

I.e. majority can vote to set nounsRecipient to zero address, securing not only all new, but all the currently outstanding streams, de facto breaking DAO and StreamEscrow isolation

Internal pre-conditions

None, nounsRecipient can be set via proposal

External pre-conditions

Rate seeking majority wins over the DAO

Attack Path

Set nounsRecipient to address(0)

Impact

All the nouns sold after the streaming was introduced with the premise of immutable minority protection will lose it. This can lead to stealing the funds from all the minority noun holders, who will be both unable either to receive any benefits from treasury (as they will be outvoted by a majority on any benefiting proposals) or to exit (as stream cancellation, being the only way to do so, will be blocked).

The value of being a minority holder and so the value of nouns can be driven down this way as minority protection is a substantial part of the current value proposition

PoC

Pass and execute the proposal with setNounsRecipient(address(0))

Mitigation

Since plain transfer is used instead of the safe version it is only zero address provides the ability to block the operation, i.e. any non-zero will not be able to block it as _checkOnERC721Received() is not called. This way it looks like forbidding zero address for nounsRecipient can be sufficient

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
nounsDAO/nouns-monorepo#871