Unchecked Return Value in `_doDeposit` Function Leads to Incorrect Assumption of Success and Potential Loss of Funds

Github username: @YowiSec Twitter username: YowiSec Submission hash (on-chain): 0x24e77e64fb763309e0e27e4d28612c7ed62eb6cbea216860ef936081c719e28e Severity: low

Description:

Description

The _doDeposit internal function in the BfxVault contract fails to check the return value of the paymentToken.approve(address(bfx), amount) call. According to the ERC20 standard, the approve function returns a boolean value indicating the success or failure of the operation. Ignoring this return value can lead to the contract incorrectly assuming that the approval succeeded even if it did not. This assumption is problematic when the subsequent bfx.deposit(amount) call is made, as it relies on the approval being successful to function correctly.

Impact

If the approve operation fails but the contract proceeds without acknowledging the failure, any mechanisms depending on the successful approval and subsequent deposit (e.g., staking mechanisms, reward calculations) may behave unexpectedly, potentially leading to loss of funds, locked assets, or a compromised state of the contract's logic. This unchecked return value poses a risk of silent failures, where the contract's state becomes inconsistent with the actual holdings and permissions, leading to operational discrepancies and security vulnerabilities.

Proof of Concept (PoC)

To see how the BfxVault contract handles a failed deposit, we must first create a scenario where the approval of a deposit fails.

Start by first adding the following code to DummyToken.sol:

bool public approveShouldFail = false;

function setApproveShouldFail(bool _approveShouldFail) external {
    approveShouldFail = _approveShouldFail;
}

// Override the approve function
function approve(address spender, uint256 amount) public virtual override returns (bool) {
    if (approveShouldFail) {
        return false;
    }
    return super.approve(spender, amount);
}

Then for the test function, add the following to BfxVault.sol:

function testMakeDepositWithUncheckedApprovalFailure() public {
    // Setup: Ensure the user has the TREASURER_ROLE
    vm.prank(_vaultOwner);
    _vault.addTreasurer(_user1);

    // Mint tokens to user1
    _token.mint(_user1, 1e18);

    // Make `approve` fail
    _token.setApproveShouldFail(true);

    // User1 attempts to approve the vault to spend their tokens (this should "fail")
    vm.prank(_user1);
    bool approveResult = _token.approve(address(_vault), 1e18);
    assertFalse(approveResult, "Approve should fail");

    // Attempt to make a deposit as user1, expecting it to fail due to approval failure
    vm.prank(_user1);
    vm.expectRevert("Token approval failed"); // Adjust based on your contract's error handling
    _vault.makeDeposit(1e18);

    // Optionally, reset the approve failure mechanism for subsequent tests
    _token.setApproveShouldFail(false);
}

Finally, run the test function with:

forge test --match-test testMakeDepositWithUncheckedApprovalFailure -vvv

Recommended Mitigation

There are a few ways to mitigate this issue including but not limited to:

Check Return Values:

Modify the _doDeposit function to check the return value of the approve call and revert if the operation fails. For example:

    require(paymentToken.approve(address(bfx), amount), "Token approval failed");

Use SafeERC20 Library:

Utilize OpenZeppelin's SafeERC20 library for token operations. SafeERC20 wraps ERC20 functions with checks that revert on failure, abstracting away the need to manually check return values and improving code readability and safety.

Incorporating these mitigations will enhance the contract's security and robustness, ensuring that the deposit operations proceed only when prerequisites are successfully met, thereby safeguarding against unintended behaviors and potential vulnerabilities.

hats-finance / Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074