emitting events without checking status of transaction can be misleading

[hats-bug-reporter[bot]]

Github username: @0xanjalit Twitter username: @pineapple_punkk Submission hash (on-chain): 0x7d7d9eb85973afe091caeaea0668353963880231ef6174ed3c637f470f6a20ae Severity: low

Description: Description\ The withdrawTokensTo function in BfxVault.sol emits event WithdrawTo(to, amount) without checking if the transaction was even successful or not

function withdrawTokensTo(uint256 amount, address to) external onlyOwner {
        require(amount > 0, "WRONG_AMOUNT");
        require(to != address(0), "ZERO_ADDRESS");
        emit WithdrawTo(to, amount); //@audit 
        bool success = _makeTransfer(to, amount);
        require(success, "TRANSFER_FAILED");
    }

same is the case with Bfx.sol:withdraw function which emits WithdrawalReceipt(id, trader, amount) which according to docs should only be emitted only if the withdrawal was successful, here: WithdrawalReceipt: Emitted after a successful trader withdrawal with signature verification

function withdraw(
        uint256 id, address trader, uint256 amount, uint8 v, bytes32 r, bytes32 s
        ) external nonReentrant {
        require(amount > 0, "WRONG_AMOUNT");
        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);//@audit
        bool success = makeTransfer(trader, amount);
        require(success, "TRANSFER_FAILED");
    }

Impact\ emitting event like this can be misleading and create disputes among frontend and actual smart contract results

Mitigation\ consider moving the emit statement down after the require statement, such that event is only emitted after satisfying the require condition. BfxVault:withdrawTokensTo function

 function withdrawTokensTo(uint256 amount, address to) external onlyOwner {
        require(amount > 0, "WRONG_AMOUNT");
        require(to != address(0), "ZERO_ADDRESS");
        // removing emit from here and shifting after require
        bool success = _makeTransfer(to, amount);
        require(success, "TRANSFER_FAILED");
        emit WithdrawTo(to, amount);
    }

Bfx:withdraw function

function withdraw(
        uint256 id, address trader, uint256 amount, uint8 v, bytes32 r, bytes32 s
        ) external nonReentrant {
        require(amount > 0, "WRONG_AMOUNT");
        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");

        //removing emit from here and shifting after require
        bool success = makeTransfer(trader, amount);
        require(success, "TRANSFER_FAILED");
        emit WithdrawalReceipt(id, trader, amount);
    }

[alex-sumner]

The checks/effects/interactions pattern would suggest events, being effects, precede transfer, which is an interaction. Regardless of the ordering chosen, no event will be emitted in the case that the transfer fails, because then the transaction will be reverted.