Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADD-03 MitigationConfirmed #10

Open
c4-bot-2 opened this issue Sep 14, 2024 · 2 comments
Open

ADD-03 MitigationConfirmed #10

c4-bot-2 opened this issue Sep 14, 2024 · 2 comments
Labels
confirmed for report This issue is confirmed for report mitigation-confirmed MR-ADD-03 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-2
Copy link
Contributor

Lines of code

Vulnerability details

See:
Pull Request

This PR is an additional change that is in scope for this mitigation review and the intention here is to prevent batch auction cancellations during final 10% of auction, this was done to help sort windows like the switch bid and bait tactics on auctions that was hinted in the main contest, see previous finding here.

This has been rightly implemented, considering now we can only cancel orders in first 90% of the auction due to the new CANCEL_WINDOW var, that is now set, and in init() the cancellationEndTime is calculated with the help of this var while initiating the Gnosis auction, see:

contracts/plugins/trading/GnosisTrade.sol{


..snip
    TradeKind public constant KIND = TradeKind.BATCH_AUCTION;
     uint256 public constant FEE_DENOMINATOR = 1000;

+     // Can only cancel order in first 90% of the auction
+     uint192 public constant CANCEL_WINDOW = 9e17; // {1} first 90% of auction

     // Upper bound for the max number of orders we're happy to have the auction clear in;
     // When we have good price information, this determines the minimum buy amount per order.
     uint96 public constant MAX_ORDERS = 5000; // bounded to avoid going beyond block gas limit
..snip

    function init(
        IBroker broker_,
        address origin_,
        IGnosis gnosis_,
        uint48 batchAuctionLength,
        TradeRequest calldata req
    ) external stateTransition(TradeStatus.NOT_STARTED, TradeStatus.OPEN) {
        require(req.sellAmount <= type(uint96).max, "sellAmount too large");
        require(req.minBuyAmount <= type(uint96).max, "minBuyAmount too large");


        sell = req.sell.erc20();
        buy = req.buy.erc20();
        sellAmount = shiftl_toFix(req.sellAmount, -int8(sell.decimals()), FLOOR); // {sellTok}


        initBal = sell.balanceOf(address(this)); // {qSellTok}
        require(initBal >= req.sellAmount, "unfunded trade");


        assert(origin_ != address(0));


        broker = broker_;
        origin = origin_;
        gnosis = gnosis_;
        endTime = uint48(block.timestamp) + batchAuctionLength;


        // D27{qBuyTok/qSellTok}
        worstCasePrice = shiftl_toFix(req.minBuyAmount, 9).divu(req.sellAmount, FLOOR);
        // cannot overflow; cannot round to 0 unless minBuyAmount is itself 0


        // Downsize our sell amount to adjust for fee
        // {qSellTok} = {qSellTok} * {1} / {1}
        uint96 _sellAmount = uint96(
            _divrnd(
                req.sellAmount * FEE_DENOMINATOR,
                FEE_DENOMINATOR + gnosis.feeNumerator(),
                FLOOR
            )
        );


        // Don't decrease minBuyAmount even if fees are in effect. The fee is part of the slippage
        uint96 minBuyAmount = uint96(Math.max(1, req.minBuyAmount)); // Safe downcast; require'd


        uint256 minBuyAmtPerOrder = Math.max(
            minBuyAmount / MAX_ORDERS,
            DEFAULT_MIN_BID.shiftl_toUint(int8(buy.decimals()))
        );


        // Gnosis EasyAuction requires minBuyAmtPerOrder > 0
        // untestable:
        //      Value will always be at least 1. Handled previously in the calling contracts.
        if (minBuyAmtPerOrder == 0) minBuyAmtPerOrder = 1;


        // == Interactions ==


        // Set allowance via custom approval -- first sets allowance to 0, then sets allowance
        // to either the requested amount or the maximum possible amount, if that fails.
        //
        // Context: wcUSDCv3 has a non-standard approve() function that reverts if the approve
        // amount is > 0 and < type(uint256).max.
        AllowanceLib.safeApproveFallbackToMax(address(sell), address(gnosis), req.sellAmount);


+         // Can only cancel within the CANCEL_WINDOW
+         uint48 cancellationEndTime = uint48(
+             block.timestamp + (batchAuctionLength * CANCEL_WINDOW) / FIX_ONE
+         );
+
        auctionId = gnosis.initiateAuction(
            sell,
            buy,
-           endTime,
+           cancellationEndTime,
            endTime,
            _sellAmount,
            minBuyAmount,
            minBuyAmtPerOrder,
            0,
            false,
            address(0),
            new bytes(0)
        );
    }
    ..snip
}

Concluding the fact that windows like the switch and bait tactics are no longer executable due to the time constraint on cancellations.

c4-bot-3 added a commit that referenced this issue Sep 14, 2024
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 18, 2024
@c4-judge
Copy link

thereksfour marked the issue as satisfactory

@c4-judge
Copy link

thereksfour marked the issue as confirmed for report

@c4-judge c4-judge added the confirmed for report This issue is confirmed for report label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed for report This issue is confirmed for report mitigation-confirmed MR-ADD-03 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants