Echo

Description

In case the last bid was made more than auctionInactivityDuration ago, the auction is considered closed:

if (    auctionInactivityDuration > 0 && lastBidTimestamp > 0        && block.timestamp >= lastBidTimestamp + auctionInactivityDuration) {    return Stage.Closed;}

Each new bid updates lastBidTimestamp with the current block timestamp. Currently, users are allowed to place a bid with the same price and amount as the previous one. Such a bid does not revert and still updates lastBidTimestamp, effectively keeping the auction open indefinitely without increasing the bid — resulting in a zero-delta bid scenario.

POC

contract GriefingPoCTest is MegaSaleTest {    // PoC: a bidder can keep the auction from auto-closing by repeatedly rebidding    // with identical price/amount/lockup, which refreshes lastBidTimestamp each time.    function testZeroDeltaRebidsKeepAuctionOpen() public {        openAuction();
        uint64 price = 10;        uint64 amount = SALE_MIN_AMOUNT; // satisfies min amount
        // initial bid        doBid({user: alice, price: price, amount: amount, lockup: false});        assertEq(uint8(sale.stage()), uint8(MegaSale.Stage.Auction));
        uint64 inactivity = sale.auctionInactivityDuration();
        // Repeat near the inactivity boundary to keep extending auction indefinitely        for (uint256 i = 0; i < 8; i++) {            // move time to just before auto-close threshold from the current last bid            vm.warp(sale.lastBidTimestamp() + inactivity - 1);            // if no activity happened now, auction would close in 1 second            assertEq(uint8(sale.stage()), uint8(MegaSale.Stage.Auction));
            // zero-delta re-bid (same price, same amount, same lockup)            doBid({user: alice, price: price, amount: amount, lockup: false});
            // lastBidTimestamp refreshed; auction remains open            assertEq(sale.lastBidTimestamp(), block.timestamp);            assertEq(uint8(sale.stage()), uint8(MegaSale.Stage.Auction));        }
        // Once the bidder stops rebidding, the auction auto-closes after the inactivity window        vm.warp(sale.lastBidTimestamp() + inactivity);        assertEq(uint8(sale.stage()), uint8(MegaSale.Stage.Closed));    }}

Recommendation

Allow new bids only if either the price or amount is increased compared to the previous bid.

Description

The codebase includes fixed-precision amount types and a permit allocation struct that are not referenced by any production contracts under src/ or by tests: Fixed6andFixed6Lib in lib/sales/Fixed6.sol, and PurchasePermitWithAllocationFixed6 in lib/sales/permits/PermitAllocation.sol. Additionally, the associated ECDSA helper functionsdigest andrecoverSigner in PermitAllocation.sol are unused. Keeping unreferenced arithmetic and cryptographic code increases maintenance overhead and security risk by broadening the potential attack surface, complicating reviews, and inviting accidental future integration without proper scrutiny or test coverage.

Recommendation

Remove the unused files and symbols to reduce surface area: delete lib/sales/Fixed6.sol and lib/sales/permits/PermitAllocation.sol, including the digest and recoverSigner functions. If this functionality is needed later or for external consumers, reintroduce it in a separate package with tests and explicit integration points.

Description

The DepositPlaced(bytes16 indexed entityID, uint256 amount)event is declared in MegaSale.solbut never emitted. In addition, withdraw() (external, onlyStage(Stage.Done)) does not emit an event. Emitting these events would improve on‑chain management and operational visibility; the project documentation indicates withdrawals should be logged, so adding a dedicated withdrawal event aligns code with documented expectations.

Recommendation

Remove DepositPlaced if deposits are not intended to be tracked, or emit it where deposits occur. Add and emit a Withdrawn (or similar) event from withdraw()to support on‑chain management and to match the documentation.

Description

If forcedLockup is set to true, then all new bids should have lockup.

if (payload.forcedLockup && !newBid.lockup) {       revert BidMustHaveLockup();}

However, it’s possible that forcedLockup is enabled after some bids have already been placed with lockup == false. If those users don’t rebid, their unlocked bids will remain valid and can still win allocations — violating the intended forced lockup rule.

Recommendation

If forcedLockup is enabled, ensure that existing bids without lockup are filtered out during the winner selection process and assigned zero allocation. Otherwise, if such behavior is supported, document it.

Description

totalActiveBidAmount is intentionally monotonic and only increases on bids, while refunds are tracked separately via totalRefundedAmount. Off-chain consumers must compute outstanding exposure as totalActiveBidAmount - totalRefundedAmount. Previous documentation implied that totalActiveBidAmount would decrease as refunds were processed, which can mislead off-chain analytics if they read the raw counter directly.

Recommendation

Document that outstanding exposure must be computed as totalActiveBidAmount - totalRefundedAmount and that totalActiveBidAmount does not decrement on refund/cancel. Optionally add a view helper (for example, outstandingActiveBidAmount()) returning the derived value to reduce integration mistakes, communicate this to integrators.