Vulnerability in Withdraw Function Allows Locking of Remaining Tokens

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @recursiveAudit Submission hash (on-chain): 0xdc4c563b13ebcebd7a558df1c5dd3a4917e84ab4e51d486623be04def381667c Severity: high

Description: Description\ In Bfx:withdraw() function, there's a vulnerability related to the handling of processed withdrawals. After processing a withdrawal request, the function sets the corresponding processedWithdrawals[id] flag to true. However, there's a missing condition to ensure that this flag is not set to true if the withdrawal amount is less than the total deposited amount, potentially locking the remaining tokens.

Attack Scenario\ Partial Withdrawal Exploitation: An attacker deposits a certain amount of tokens into the contract. Then, they initiate a withdrawal request for a portion of the deposited amount, for example, withdrawing only half of the deposited tokens.

Flag Manipulation: Due to the vulnerability, the processedWithdrawals[id] flag is set to true after processing the withdrawal request, regardless of whether the entire deposited amount is withdrawn or not.

Remaining Tokens Locked: Since the flag is set to true after the partial withdrawal, subsequent attempts by the trader to withdraw the remaining tokens will fail. The require(processedWithdrawals[id] == false, "ALREADY_PROCESSED") check will prevent further withdrawals, leading to the remaining tokens being locked in the contract.

Denial of Service (DoS): The attacker effectively denies the trader access to their remaining tokens by exploiting the vulnerability, potentially causing financial losses and undermining the integrity of the system.

Attachments

1. Proof of Concept (PoC) File

//withdraw function
    function withdraw(
        ) external nonReentrant {
        require(amount > 0, "WRONG_AMOUNT");
        // processwithdraw check shd be false and set to true,
        // not handling properly
        require(processedWithdrawals[id] == false, "ALREADY_PROCESSED");
        processedWithdrawals[id] = true;
        bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(
            keccak256("withdrawal(uint256 id,address trader,uint256 amount)"),
            id,
            trader,
            amount
        )));

        bool valid = verify(digest, v, r, s);
        require(valid, "INVALID_SIGNATURE");

        emit WithdrawalReceipt(id, trader, amount);
        bool success = makeTransfer(trader, amount);
        require(success, "TRANSFER_FAILED");
    }

This minimal code example can be run on remix.ide and see once sendtransfer:withdrawal is executed by trader again sendtransfer:withdraw function cannot be called, the count which increases on the execution will not increase more than one.

//SPDX-License-Identifier:MIT
pragma solidity 0.8.23;

contract sendtransfer {
uint256 public count;
        mapping(uint256 => bool) public processedWithdrawals;
        function deposit( uint256 id,uint256 amount,
        address trader
        ) external  {
        require(amount > 0, "WRONG_AMOUNT");
        require(processedWithdrawals[id] == false, "ALREADY_PROCESSED");
        processedWithdrawals[id] = true;
        count++;

        }
}

2. Revised Code File (Optional) check for the cases whether trader have any remaining tokens or not.

[alex-sumner]

The flag on processed withdrawal ids ensures that a particular withdrawal can only be processed once. It does not prevent subsequent withdrawals by the same user or others. Any subsequent withdrawals will have a different, and unused, id.

[recursiveEth]

@alex-sumner The bfx:deposit() function deposit the user specified amount, as there is no mapping for how much a user has deposited which helps in withdrawing the tokens, everything is dependent on event which log depositId and amount.

    function deposit(uint256 amount) external nonReentrant {
        bool success = makeTransferFrom(msg.sender, address(this) , amount);
        require(success, "TRANSFER_FAILED");
        uint256 depositId = allocateDepositId();
        emit Deposit(depositId, msg.sender, amount);
    }

so by providing amount as an input parameter in bfx:withdraw()function, it allows user to withdraw amount of there choice , otherwise it should implicitly calculate the total amount related to that depositId, i hope it's a valid issue. thank you

[alex-sumner]

The amount anyone can withdraw is determined by the exchange. It is not simply the amount that the person earlier deposited, since they can have made or lost money trading since they made their deposit. You cannot withdraw any amount you want simply by calling withdraw, each request needs to be accompanied by a valid signature. You can only obtain a signature for an amount up to your current withdrawable balance. Once you have a signature for an amount you can only use it once, and only to withdraw that amount and only to your wallet. The signature is a based on a hash of various information including the amount, if you obtain a signature for one amount and then try to use it for another amount your transaction will be reverted with the error "INVALID_SIGNATURE".

[recursiveEth]

thanks for explaining.