Skip to content

Latest commit

 

History

History
128 lines (95 loc) · 5.71 KB

072.md

File metadata and controls

128 lines (95 loc) · 5.71 KB

Sweet Infrared Snake

High

Lack of access control for multiple stream cancellations in cancelStreams function allow unauthorized users to cancel streams of Noun tokens they do not own in StreamEscrow.sol

Summary

The cancelStreams function in the contract allows a user to cancel multiple streams for Nouns at once. However, it lacks proper access control to ensure that only the owner of the Noun token or an authorized user can cancel streams for multiple Noun tokens. This allows unauthorized users to cancel streams of Noun tokens they do not own.

Root Cause

The cancelStreams function allows for multiple streams to be canceled in a single transaction: https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/StreamEscrow.sol#L155C5-L159

/**
 * @notice Cancels multiple streams at once.
 * @param nounIds The IDs of the Noun tokens to cancel streams for.
 */
function cancelStreams(uint256[] calldata nounIds) external {
    for (uint256 i; i < nounIds.length; ++i) {
        cancelStream(nounIds[i]);
    }
}

In this function, there is no additional access control or validation on the caller’s permissions for each nounId in the nounIds array. Although the individual cancelStream function verifies ownership of the Noun token, the cancelStreams function does not re-check if the caller is authorized to cancel the streams for the specific tokens in the batch.

This oversight means that if an unauthorized user can pass a list of nounIds in the nounIds array, they could cancel streams for Nouns they don’t own, causing unintended stream cancellations.

Same with cancelStream function. There is no access control to ensure that only the owner of the Noun token or an approved address can call the cancelStream function. The contract only checks whether the stream is active (isStreamActive(nounId)), but does not verify if the caller is the Noun token owner or an approved operator.

This means that any address (even someone who does not own the Noun token) can call the cancelStream function and cancel the stream, transferring the Noun token and potentially stealing funds.

So, a malicious user can call cancelStream with a Noun ID that does not belong to them, causing the Noun token to be transferred to the nounsRecipient and ETH to be refunded to the attacker.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

A malicious actor can exploit the vulnerability by calling cancelStreams with a list of nounIds that they do not own. This allows them to cancel streams for tokens they are not authorized to touch.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

interface IStreamEscrow {
    function cancelStreams(uint256[] calldata nounIds) external;
}

contract MaliciousContract {
    IStreamEscrow public escrow;
    address public owner;

    constructor(address _escrow) {
        escrow = IStreamEscrow(_escrow);
        owner = msg.sender;
    }

    function attack(uint256[] calldata nounIds) public {
        escrow.cancelStreams(nounIds);
    }
}
  1. The attacker deploys a contract MaliciousContract and calls the attack function with a list of Noun IDs (nounIds) they do not own.
  2. The cancelStreams function loops through the list and calls cancelStream for each Noun ID without checking if the caller owns or is authorized to cancel the stream.
  3. As a result, the attacker is able to cancel streams for tokens they don't own.

Impact

  1. The most immediate and severe impact of this vulnerability is that unauthorized users can cancel streams for Nouns they do not own, which will lead to financial loss for the legitimate owner and potential abuse of the system.
  2. If Noun owners rely on the correct functioning of the streaming system, unauthorized cancellations could lead to loss of funds, as streams may be prematurely stopped or canceled.

PoC

To test this, deploy a mock contract to simulate the cancelStreams functionality and then test the behavior when a malicious contract calls it.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import { Test } from "forge-std/Test.sol";
import { IStreamEscrow } from "../StreamEscrow.sol";

contract StreamEscrowTest is Test {
    IStreamEscrow escrow;

    address public maliciousAddress;

    function setUp() public {
        // Deploy the StreamEscrow contract
        escrow = new IStreamEscrow();
        
        // Simulate the malicious actor's address
        maliciousAddress = address(new MaliciousContract(address(escrow)));
    }

    function testCancelStreams() public {
        uint256;
        invalidNounIds[0] = 1;
        invalidNounIds[1] = 2;

        // Execute the attack
        vm.prank(maliciousAddress); // Pretend to be the malicious actor
        escrow.cancelStreams(invalidNounIds);
    }
}

Mitigation

To mitigate the issue, you should add proper access control checks to ensure that only the owner of a Noun or an authorized party (like a DAO or operator) can cancel streams for that Noun token. This can be done by re-checking ownership or permission for each nounId in the cancelStreams function.

/**
 * @notice Cancels multiple streams at once, with proper access control.
 * @param nounIds The IDs of the Noun tokens to cancel streams for.
 */
function cancelStreams(uint256[] calldata nounIds) external {
    for (uint256 i; i < nounIds.length; ++i) {
        require(isApprovedOrOwner(msg.sender, nounIds[i]), "not noun owner or approved");
        cancelStream(nounIds[i]);
    }
}

In this modified version, the isApprovedOrOwner function ensures that only the Noun owner or an approved operator can cancel the stream for each nounId in the nounIds array.