036.md

Nice Basil Panda

Medium

Client will receive more rewards than they suppose to when winner cancel stream

Summary

Rewards for clients distributed based of revenue they brought, but revenue is not being changed when buyer cancel his purchase

Root Cause

Here is a function that ditribute rewards which uses getSettlements from auctionHouse

    function updateRewardsForAuctions(uint32 lastNounId) public whenNotPaused {
        uint256 startGas = gasleft();
        RewardsStorage storage $ = _getRewardsStorage();
        if (!$.auctionRewardsEnabled) revert RewardsDisabled();

        bool sawValidClientId = false;
        uint256 nextAuctionIdToReward_ = $.nextAuctionIdToReward;
        if (lastNounId < nextAuctionIdToReward_ + $.auctionRewardParams.minimumAuctionsBetweenUpdates)
            revert LastNounIdMustBeHigher();

        $.nextAuctionIdToReward = lastNounId + 1;

-->      INounsAuctionHouseV2.Settlement[] memory settlements = auctionHouse.getSettlements(
            nextAuctionIdToReward_,
            lastNounId + 1,
            true
        );
        INounsAuctionHouseV2.Settlement memory lastSettlement = settlements[settlements.length - 1];
        if (!(lastSettlement.nounId == lastNounId && lastSettlement.blockTimestamp > 1))
            revert LastNounIdMustBeSettled();

        uint32 maxClientId = nextTokenId() - 1;
        ClientRewardsMemoryMapping.Mapping memory m = ClientRewardsMemoryMapping.createMapping({
            maxClientId: maxClientId
        });

        for (uint256 i; i < settlements.length; ++i) {
            INounsAuctionHouseV2.Settlement memory settlement = settlements[i];
            if (settlement.clientId != 0 && settlement.clientId <= maxClientId) {
                sawValidClientId = true;
-->                m.inc(settlement.clientId, settlement.amount);
            }
        }
...

client-incentives/Rewards.sol#L255

    function getSettlements(
        uint256 startId,
        uint256 endId,
        bool skipEmptyValues
    ) external view returns (Settlement[] memory settlements) {
...
        for (uint256 id = startId; id < endId; ++id) {
            settlementState = settlementHistory[id];

            if (skipEmptyValues && settlementState.blockTimestamp <= 1) continue;

            settlements[actualCount] = Settlement({
                blockTimestamp: settlementState.blockTimestamp,
-->                amount: uint64PriceToUint256(settlementState.amount),
                winner: settlementState.winner,
                nounId: id,
                clientId: settlementState.clientId
            });
            ++actualCount;
        }

contracts/NounsAuctionHouseV2.sol#L516 But client reward does decrease when user rage quit via cancel stream, since he withdraw some funds. But client's revenue is not being updated

    function cancelStream(uint256 nounId) public {
        require(isStreamActive(nounId), 'stream not active');

        // transfer noun to treasury
        nounsToken.transferFrom(msg.sender, nounsRecipient, nounId);

        // cancel stream
        streams[nounId].canceled = true;
        Stream memory stream = streams[nounId];
        ethStreamedPerTick -= stream.ethPerTick;
        ethStreamEndingAtTick[stream.lastTick] -= stream.ethPerTick;

        // calculate how much needs to be refunded
        uint256 ticksLeft = stream.lastTick - currentTick;
        uint256 amountToRefund = stream.ethPerTick * ticksLeft;
        (bool sent, ) = msg.sender.call{ value: amountToRefund }('');
        require(sent, 'failed to send eth');

        emit StreamCanceled(nounId, amountToRefund, ethStreamedPerTick);
    }

StreamEscrow.sol#L167

Internal pre-conditions

2 scenarios

1. user rageQuit
2. settlement.amount * auctionRewardBps) / 10_000 > (_auction.amount * immediateTreasuryBPs) / 10_000

External pre-conditions

No response

Attack Path

Whenever this condition met settlement.amount * auctionRewardBps) / 10_000 > (_auction.amount * immediateTreasuryBPs) / 10_000 The user buys nft for (_auction.amount * immediateTreasuryBPs) / 10_000 immediately returns the NFT, later receives settlement.amount * auctionRewardBps) / 10_000 rewards

Impact

There are two impacts:

1. Rewards are not being distributed fairly across clients whenever user ragequit due to client revenue not changing
2. under certain internal conditions, clients can steal funds from system: (settlement.amount * auctionRewardBps) / 10_000 > (_auction.amount * immediateTreasuryBPs) / 10_000

PoC

No response

Mitigation

I think its should be the longer user vest the less rewards he should earn. If he forwards, he receives more rewards.

        SettlementState storage settlementState = settlementHistory[_auction.nounId];
        settlementState.blockTimestamp = uint32(block.timestamp);
-        settlementState.amount = ethPriceToUint64(_auction.amount);
+        settlementState.amount = ethPriceToUint64(amountToSendTreasury);
        settlementState.winner = _auction.bidder;
        if (_auction.clientId > 0) settlementState.clientId = _auction.clientId;

        emit AuctionSettled(_auction.nounId, _auction.bidder, _auction.amount);
        if (_auction.clientId > 0) emit AuctionSettledWithClientId(_auction.nounId, _auction.clientId);
    }

+    function increaseSettlement(uint nounId, uint amoun) external onlyEscrow {
+        SettlementState storage settlementState = settlementHistory[nounId];
+        settlementState.amount = settlementState.amount + ethPriceToUint64(amoun);
+    }

    function fastForwardStream(uint256 nounId, uint32 ticksToForward) public {
        require(ticksToForward > 0, 'ticksToForward must be positive');
        require(nounsToken.ownerOf(nounId) == msg.sender, 'not noun owner');

        Stream memory stream = streams[nounId];
        uint32 currentTick_ = currentTick;
        require(isStreamActive(stream, currentTick_), 'stream not active');

        // move last tick
        require(ticksToForward <= stream.lastTick - currentTick_, 'ticksToFoward too large');
        uint32 newLastTick = stream.lastTick - ticksToForward;

        ethStreamEndingAtTick[stream.lastTick] -= stream.ethPerTick;
        streams[nounId].lastTick = newLastTick;

        if (newLastTick > currentTick_) {
            // stream is still active, so register the new end tick
            ethStreamEndingAtTick[newLastTick] += stream.ethPerTick;
        } else {
            // no more ticks left, so finished the stream
            ethStreamedPerTick -= stream.ethPerTick;
        }

        uint256 ethToStream = ticksToForward * stream.ethPerTick;
        sendETHToTreasury(ethToStream);
+        auctionHouse.increaseSettlement(nounId, ethToStream);
        emit StreamFastForwarded(nounId, ticksToForward, newLastTick, ethStreamedPerTick);
    }