Skip to content

Latest commit

 

History

History
223 lines (164 loc) · 7.43 KB

067.md

File metadata and controls

223 lines (164 loc) · 7.43 KB
Raw

Electric Marigold Cobra

Medium

Loss of Funds Due to Missing Address Validation in StreamEscrow

Summary

A vulnerability was identified in the initialization and configuration functions of the StreamEscrow contract. The absence of validation for critical variables (daoExecutor, ethRecipient, nounsRecipient) allows assigning invalid addresses, including address(0). This issue could lead to potential loss of funds and operational instability. A mitigation was implemented by enforcing strict address validation, ensuring that these variables cannot be set to address(0). Comprehensive testing confirmed the effectiveness of this solution, preventing invalid configurations.

Vulnerability Details

The StreamEscrow contract lacks validation to prevent critical addresses (daoExecutor, ethRecipient, nounsRecipient) from being set to address(0) during both initialization and updates. This flaw could lead to operational failures, loss of funds, or contract misbehavior.

Impact:

  • Loss of Funds: ETH intended for ethRecipient or daoExecutor could become irretrievable if sent to address(0).
  • Operational Instability: Assigning address(0) disrupts critical contract functions, including fund streaming and governance processes.
  • Governance Exploitation: Malicious or negligent actors within the DAO could intentionally or accidentally set invalid addresses, undermining the protocol’s reliability.

Code Analysis

https://github.com/sherlock-audit/2024-11-nounsdao/blob/8b6fb94f103134e751cf016e5c3f4185be89bb49/nouns-monorepo/packages/nouns-contracts/contracts/StreamEscrow.sol#L76

Constructor and Setter Function Excerpts:

Constructor:

constructor(
    address daoExecutor_,
    address ethRecipient_,
    address nounsRecipient_,
    address nounsToken_,
    address streamCreator_,
    uint32 minimumTickDuration_
) {
    daoExecutor = daoExecutor_;
    ethRecipient = ethRecipient_;
    nounsRecipient = nounsRecipient_;
    nounsToken = INounsToken(nounsToken_);
    allowedToCreateStream[streamCreator_] = true;
    minimumTickDuration = minimumTickDuration_;
}

Setter Example:

function setETHRecipient(address newAddress) external onlyDAO {
    ethRecipient = newAddress;
    emit ETHRecipientSet(newAddress);
}

Key Issue:

  • No Validation: Both the constructor and setter functions lack safeguards to ensure that critical addresses are not set to address(0) or other invalid values. This could result in address(0) being unintentionally or maliciously assigned.

Proof of Concept (PoC)

Original Test Demonstrating the Vulnerability

The following tests highlight the ability to set address(0) without triggering any errors or reverts:

Constructor:

function test_constructor_allowsZeroAddresses() public {
    StreamEscrow newEscrow = new StreamEscrow(
        address(0), // daoExecutor
        address(0), // ethRecipient
        address(0), // nounsRecipient
        address(nounsToken),
        streamCreator,
        24 hours
    );

    assertEq(newEscrow.daoExecutor(), address(0), "DAOExecutor should be address(0)");
    assertEq(newEscrow.ethRecipient(), address(0), "ETHRecipient should be address(0)");
    assertEq(newEscrow.nounsRecipient(), address(0), "NounsRecipient should be address(0)");
}

Setter Functions:

function test_setETHRecipient_toZeroAddress() public {
    vm.prank(treasury);
    escrow.setETHRecipient(address(0));
    assertEq(escrow.ethRecipient(), address(0), "ETHRecipient should be address(0)");
}

Test Results:

[PASS] test_constructor_allowsZeroAddresses() (gas: 1558719)
[PASS] test_setETHRecipient_toZeroAddress() (gas: 15812)

These tests confirmed that address(0) could be successfully assigned without validation.

Mitigated Implementation

The vulnerability was mitigated by introducing validation in the constructor and setter functions to ensure that critical variables cannot be set to address(0).

Mitigation Steps:

  1. Add Validation in the Constructor:

    constructor(
    address daoExecutor_,
    address ethRecipient_,
    address nounsRecipient_,
    address nounsToken_,
    address streamCreator_,
    uint32 minimumTickDuration_
) {
    require(daoExecutor_ != address(0), "Invalid DAO executor address");
    require(ethRecipient_ != address(0), "Invalid ETH recipient address");
    require(nounsRecipient_ != address(0), "Invalid Nouns recipient address");
    require(nounsToken_ != address(0), "Invalid Nouns token address");
    require(streamCreator_ != address(0), "Invalid stream creator address");

    daoExecutor = daoExecutor_;
    ethRecipient = ethRecipient_;
    nounsRecipient = nounsRecipient_;
    nounsToken = INounsToken(nounsToken_);
    allowedToCreateStream[streamCreator_] = true;
    minimumTickDuration = minimumTickDuration_;
}

  2. Enforce Validation in Setter Functions:

    function setETHRecipient(address newAddress) external onlyDAO {
    require(newAddress != address(0), "Invalid ETH recipient address");
    ethRecipient = newAddress;
    emit ETHRecipientSet(newAddress);
}

Mitigated Test Code

Updated tests ensure that attempts to set address(0) revert as expected:

function test_constructor_revertsWithZeroAddresses() public {
    vm.expectRevert("Invalid DAO executor address");
    new StreamEscrow(
        address(0),
        ethRecipient,
        nounsRecipient,
        address(nounsToken),
        streamCreator,
        24 hours
    );

    vm.expectRevert("Invalid ETH recipient address");
    new StreamEscrow(
        daoExecutor,
        address(0),
        nounsRecipient,
        address(nounsToken),
        streamCreator,
        24 hours
    );
}

function test_setETHRecipient_toZeroAddress() public {
    vm.prank(treasury);
    vm.expectRevert("Invalid ETH recipient address");
    escrow.setETHRecipient(address(0));
}

Test Results:

[PASS] test_constructor_revertsWithZeroAddresses() (gas: 153432)
[PASS] test_setETHRecipient_toZeroAddress() (gas: 13129)

The tests confirmed that the mitigation successfully prevents assigning address(0).

Impact

  • High Severity: Assigning address(0) disrupts fund streaming, governance, and treasury operations, potentially leading to loss of funds and protocol instability.
  • Governance Integrity: The mitigation ensures malicious or negligent actions within the DAO cannot compromise critical addresses.

Tools Used

  • Foundry: Comprehensive testing framework for fuzz and unit testing.
  • Manual Code Review: Detailed analysis of constructor and setter logic.

Recommendations

  1. Strict Validation: Continue enforcing address(0) checks for all critical address setters.
  2. Comprehensive Tests: Ensure tests cover edge cases for address validation in all configurable parameters.
  3. Governance Audits: Regularly review governance proposals to prevent malicious or erroneous changes.

Conclusion

The StreamEscrow contract initially allowed critical addresses to be set to address(0) due to missing validation. This vulnerability was mitigated by adding robust checks in the constructor and setter functions, effectively preventing invalid configurations. Updated tests confirm the solution is effective, ensuring operational stability and fund security.