Merge pull request #873 from nounsDAO/verbs-stream-escrow-audit-fixes

StreamEscrow fixes post sherlock audit
nounsDAO · Dec 12, 2024 · adede58 · adede58
2 parents 41c3b43 + 8df901f
commit adede58
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 6 deletions.
diff --git 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');
         _;
@@ -73,6 +76,8 @@ contract StreamEscrow is IStreamEscrow {
         address streamCreator_,
         uint32 minimumTickDuration_
     ) {
+        require(nounsRecipient_ != address(0), 'zero address');
+
         daoExecutor = daoExecutor_;
         ethRecipient = ethRecipient_;
         nounsRecipient = nounsRecipient_;
@@ -119,14 +124,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 +146,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_);
     }
 
     /**
@@ -293,8 +300,10 @@ 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');
         nounsRecipient = newAddress;
         emit NounsRecipientSet(newAddress);
     }
@@ -306,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);
     }
 
     /**

diff --git 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 {