If a user/trader partially withdraws their deposit, they will never be able to withdraw the remainder

[hats-bug-reporter[bot]]

Github username: -- Twitter username: iamandreiski Submission hash (on-chain): 0x4d3dd16e9837b7214cf487d79324bd81cec62cd09fdd28dce3acf04e790dc53c Severity: medium

Description: Description\

Making the assumption that the protocol keeps tracks of balances, trades and deposit ids off-chain, a user which has deposited, and a certain "id" was assigned to that deposit, in case they only withdraw a portion of it, the remainder will be stuck in the contract due to processedWithdrawals and would have to be manually extracted by admin.

Attack Scenario\

• Imagine that Bob has made a deposit. That deposit will get its own depositId (i.e. nextDepositId variable), let's say that Bob deposited 100 units of a certain token.
• Bob decides to withdraw 50 units of his deposit, once he withdraws using the depositID which he got during the deposit, Bfx.withdraw will record the id under processedWithdrawals[id] as true.
• Bob decides to withdraw the remaining 50 units of his token, this will always fail due to the above-mentioned mapping of that id being set to true.

Here is the code in question part of the Bfs.sol, withdraw() function:

 require(processedWithdrawals[id] == false, "ALREADY_PROCESSED");
        processedWithdrawals[id] = true;

Once the first deposit is successful and the deposit id used is set as true, the withdraw function will keep reverting with "ALREADY_PROCESSED" reason.

This is marked as a medium since it's still possible for an admin to manually withdraw and send funds to users.

[JacoboLansac]

I think it is valid, but could be extended. The edge case is there for any time a user wants to withdraw twice the same amount.

• deposit 100
• withdraw 10 - ok
• withdraw 10 - reverts

[iamandreiski]

Thanks for the input, also here is a PoC that can be inserted into BfxSigning.t.sol to confirm this:

    function testRevertRepeatClaim() public {
        uint256 id1 = 55;

        //First withdrawal sigUtils
        SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({id: id1, trader: _claimant, amount: 20_000});

        //Second withdrawal sigUtils (same id, different amount)
        SigUtils.Withdrawal memory withdrawal1 = SigUtils.Withdrawal({id: id1, trader: _claimant, amount: 25_000});

        bytes32 digest = _sigUtils.getTypedDataHash(withdrawal);

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(_settlerPrivateKey, digest);

        _bfx.withdraw(withdrawal.id, withdrawal.trader, withdrawal.amount, v, r, s);

        //Preparing second Withdrawal

        bytes32 digest1 = _sigUtils.getTypedDataHash(withdrawal1);

        (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(_settlerPrivateKey, digest1);

        vm.expectRevert("ALREADY_PROCESSED");
        _bfx.withdraw(withdrawal.id, withdrawal.trader, withdrawal.amount, v1, r1, s1);
    }

[alex-sumner]

The withdrawal id check prevents the same withdrawal from being processed more than once - it prevents replay attacks. If you deposit 20 tokens you can withdraw 10 and then another 10, but in order to make 2 withdrawals you have to make 2 withdrawal requests. Each new withdrawal will be assigned a new id and a new signature.