From a1d38e38c1dc8b5608c8e090c1fb390ba38ff1f2 Mon Sep 17 00:00:00 2001 From: davidbrai Date: Tue, 3 Dec 2024 11:50:05 +0000 Subject: [PATCH 1/4] don't allow nounsRecipient to be zero address this will cause cancels to revert --- packages/nouns-contracts/contracts/StreamEscrow.sol | 2 ++ .../nouns-contracts/test/foundry/StreamEscrow.t.sol | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/packages/nouns-contracts/contracts/StreamEscrow.sol b/packages/nouns-contracts/contracts/StreamEscrow.sol index 5acd8a077..ed0185666 100644 --- a/packages/nouns-contracts/contracts/StreamEscrow.sol +++ b/packages/nouns-contracts/contracts/StreamEscrow.sol @@ -75,6 +75,7 @@ contract StreamEscrow is IStreamEscrow { ) { daoExecutor = daoExecutor_; ethRecipient = ethRecipient_; + require(nounsRecipient_ != address(0), 'zero address'); nounsRecipient = nounsRecipient_; nounsToken = INounsToken(nounsToken_); allowedToCreateStream[streamCreator_] = true; @@ -279,6 +280,7 @@ contract StreamEscrow is IStreamEscrow { * @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 { + require(newAddress != address(0), 'zero address'); nounsRecipient = newAddress; emit NounsRecipientSet(newAddress); } diff --git a/packages/nouns-contracts/test/foundry/StreamEscrow.t.sol b/packages/nouns-contracts/test/foundry/StreamEscrow.t.sol index 99d7e1666..6e8dc5f58 100644 --- a/packages/nouns-contracts/test/foundry/StreamEscrow.t.sol +++ b/packages/nouns-contracts/test/foundry/StreamEscrow.t.sol @@ -693,6 +693,17 @@ contract DAOSettersTest is BaseStreamEscrowTest { // check that new recipient received the noun assertEq(nounsToken.ownerOf(1), makeAddr('nounsRecipient2')); } + + function test_setNounsRecipient_cantBeZero() public { + vm.prank(treasury); + vm.expectRevert('zero address'); + escrow.setNounsRecipient(address(0)); + } + + function test_nounsReceipient_cantBeZeroInConstructor() public { + vm.expectRevert('zero address'); + new StreamEscrow(treasury, ethRecipient, address(0), address(nounsToken), streamCreator, 24 hours); + } } contract RescueTokensTest is BaseStreamEscrowTest { From eaab477a384a8ad30bac7fbfcd4de2ab8c662d0d Mon Sep 17 00:00:00 2001 From: davidbrai Date: Tue, 3 Dec 2024 15:37:37 +0000 Subject: [PATCH 2/4] minor refactor and natspec --- packages/nouns-contracts/contracts/StreamEscrow.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/nouns-contracts/contracts/StreamEscrow.sol b/packages/nouns-contracts/contracts/StreamEscrow.sol index ed0185666..224735701 100644 --- a/packages/nouns-contracts/contracts/StreamEscrow.sol +++ b/packages/nouns-contracts/contracts/StreamEscrow.sol @@ -73,9 +73,10 @@ contract StreamEscrow is IStreamEscrow { address streamCreator_, uint32 minimumTickDuration_ ) { + require(nounsRecipient_ != address(0), 'zero address'); + daoExecutor = daoExecutor_; ethRecipient = ethRecipient_; - require(nounsRecipient_ != address(0), 'zero address'); nounsRecipient = nounsRecipient_; nounsToken = INounsToken(nounsToken_); allowedToCreateStream[streamCreator_] = true; @@ -278,6 +279,7 @@ contract StreamEscrow is IStreamEscrow { /** * @notice Allows the DAO to set the address that the Nouns tokens will be sent to when streams are canceled. + * The zero address is not allowed because it will cause the transfer to revert, which will cause all cancellations to revert. */ function setNounsRecipient(address newAddress) external onlyDAO { require(newAddress != address(0), 'zero address'); From d9ec4b595c08cd1bd46b413275a25c7aaaffafef Mon Sep 17 00:00:00 2001 From: davidbrai Date: Wed, 4 Dec 2024 16:15:10 +0000 Subject: [PATCH 3/4] refactor to follow checks-effects-interactions --- packages/nouns-contracts/contracts/StreamEscrow.sol | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/nouns-contracts/contracts/StreamEscrow.sol b/packages/nouns-contracts/contracts/StreamEscrow.sol index d174841d2..37d5ba4f3 100644 --- a/packages/nouns-contracts/contracts/StreamEscrow.sol +++ b/packages/nouns-contracts/contracts/StreamEscrow.sol @@ -119,14 +119,14 @@ contract StreamEscrow is IStreamEscrow { uint32 streamLastTick = currentTick + streamLengthInTicks; ethStreamEndingAtTick[streamLastTick] += ethPerTick; - // the remainder is immediately streamed to the DAO - uint256 remainder = msg.value % streamLengthInTicks; - sendETHToTreasury(remainder); - uint128 newEthStreamedPerTick = ethStreamedPerTick + ethPerTick; ethStreamedPerTick = newEthStreamedPerTick; streams[nounId] = Stream({ ethPerTick: ethPerTick, canceled: false, lastTick: streamLastTick }); emit StreamCreated(nounId, msg.value, streamLengthInTicks, ethPerTick, newEthStreamedPerTick, streamLastTick); + + // the remainder is immediately streamed to the DAO + uint256 remainder = msg.value % streamLengthInTicks; + sendETHToTreasury(remainder); } /** @@ -141,11 +141,13 @@ contract StreamEscrow is IStreamEscrow { lastForwardTimestamp = toUint48(block.timestamp); - sendETHToTreasury(ethStreamedPerTick); + uint128 ethStreamedPerTick_ = ethStreamedPerTick; (uint32 newTick, uint128 ethPerTickEnded) = increaseTicksAndFinishStreams(); emit StreamsForwarded(newTick, ethPerTickEnded, ethStreamedPerTick, lastForwardTimestamp); + + sendETHToTreasury(ethStreamedPerTick_); } /** From 8df901fea1c9e2d44ead93af86accf583bd6e7e8 Mon Sep 17 00:00:00 2001 From: davidbrai Date: Wed, 11 Dec 2024 11:53:40 +0000 Subject: [PATCH 4/4] use safeTransfer when rescuing tokens --- packages/nouns-contracts/contracts/StreamEscrow.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/nouns-contracts/contracts/StreamEscrow.sol b/packages/nouns-contracts/contracts/StreamEscrow.sol index 65f722baa..e183e737d 100644 --- a/packages/nouns-contracts/contracts/StreamEscrow.sol +++ b/packages/nouns-contracts/contracts/StreamEscrow.sol @@ -20,8 +20,11 @@ pragma solidity ^0.8.19; import { IStreamEscrow } from './interfaces/IStreamEscrow.sol'; import { INounsToken } from './interfaces/INounsToken.sol'; import { IERC20 } from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; +import { SafeERC20 } from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; contract StreamEscrow is IStreamEscrow { + using SafeERC20 for IERC20; + modifier onlyDAO() { require(msg.sender == daoExecutor, 'only dao'); _; @@ -312,7 +315,7 @@ contract StreamEscrow is IStreamEscrow { * @param amount The amount of tokens to rescue. */ function rescueToken(address token, address to, uint256 amount) external onlyDAO { - IERC20(token).transfer(to, amount); + IERC20(token).safeTransfer(to, amount); } /**