Skip to content

Latest commit

 

History

History
72 lines (55 loc) · 4.08 KB

073.md

File metadata and controls

72 lines (55 loc) · 4.08 KB

Brief Honey Penguin

Medium

Manual Execution Requirement in forwardAll() Leading to Delayed and Missed Funds Forwarding

Summary

The function forwardAll() must be manually triggered to forward ETH to the treasury at regular intervals (based on minimumTickDuration). However, if the function is not called in a timely manner, it can cause a significant delay in forwarding funds. https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/StreamEscrow.sol#L136-L149 The logic in forwardAll():

 // silently fail if at least a day hasn't passed. this is in order not to revert auction house.
        if (block.timestamp < lastForwardTimestamp + minimumTickDuration) {
            return;
        }

ensures that the function can only be successfully called if a day or more has passed since lastForwardTimestamp. If forwardAll() is called late, it will only forward funds once, and the function will then require another full time interval to pass before it can be called again. This means any missed periods will not be automatically forwarded, causing a delay in treasury funds and missed tick intervals.

Ideally, the NounsAuctionHouseV3::settleCurrentAndCreateNewAuction() calls an internal function _settleAuction() to conclude the auction, send the funds to the stream escrow and call the forwardAllAndCreateStream() which calls forwardAll() and starts a new stream, if there is a bid (bid amount to stream) else the _settleAuction() just calls forwardAll() on the stream escrow. if for some reason the NounsAuctionHouse is unable to settle the current auction which has lasted for more than a day, forwardAll() will have to be triggered manually which will cause delay and missed tick intervals.

Impact

If forwardAll() is not called exactly on time, any missed funds will not be forwarded until the function is manually triggered. The forwarding mechanism depends entirely on someone/NounsAuctionHouse calling forwardAll(), and if there is any lapse in forwarding, the funds are delayed.

PoC

copy and paste this in CreateStreamPermissionsTest

  function test_ForwardAll_MissedTicksIgnored() public {
        vm.prank(streamCreator);
        escrow.forwardAllAndCreateStream{ value: 10e18 }(1, 1000);
        vm.warp(block.timestamp + 48 hours); // 2 day since the last forwardall called
        escrow.forwardAll(); // calling it will only forward once
        // reseting the lastForwardTimestamp, making us unable to forward the remaining
        assertEq(address(ethRecipient).balance, 1 * (10e18 / 1000));
    }

then run the test forge test --mt test_ForwardAll_MissedTicksIgnored

Mitigation

Allow forwardAll() to accumulate missed intervals and forward funds accordingly. Calculate how many full minimumTickDuration periods have passed since lastForwardTimestamp, and iterate to forward ETH to treasury for each missed tick

 function forwardAllMitigated() public {
        // silently fail if at least a day hasn't passed. this is in order not to revert auction house
        while (block.timestamp >= lastForwardTimestamp + minimumTickDuration) {
            lastForwardTimestamp += minimumTickDuration;
            sendETHToTreasury(ethStreamedPerTick);
            (uint32 newTick, uint128 ethPerTickEnded) = increaseTicksAndFinishStreams();
            emit StreamsForwarded(newTick, ethPerTickEnded, ethStreamedPerTick, lastForwardTimestamp);
        }
    }

POC to test the mitigation

  function test_ForwardAll_MissedTicksIgnored_Mitigated() public {
        vm.prank(streamCreator);
        escrow.forwardAllAndCreateStream{ value: 10e18 }(1, 1000);
        vm.warp(block.timestamp + 48 hours); // 2 day since the last forwardall called
        escrow.forwardAllMitigated(); // calling it will only forward twice
        //forwarding the correct number of ticks
        assertEq(address(ethRecipient).balance, 2 * (10e18 / 1000));
    }

copy and paste this in CreateStreamPermissionsTest then run the test forge test --mt test_ForwardAll_MissedTicksIgnored_Mitigated