`CompoundVeFNXManagedNFTStrategyUpgradeable` is vulnerable to a race condition

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

Description: https://github.com/hats-finance/Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d/blob/353c8e8e24454336e805e5c0e11e4e9ae1491d03/contracts/nest/CompoundVeFNXManagedNFTStrategyUpgradeable.sol#l129-L139

Description

The ERC20 approval mechanism in CompoundVeFNXManagedNFTStrategyUpgradeable is vulnerable to a race condition. This vulnerability occurs when an allowance is changed directly from one non-zero value to another non-zero value. An attacker can exploit this situation by front-running transactions, leading to unintended token transfers.

Attack Scenario

Imagine a scenario where:

Alice initially approves a spender to spend 100 tokens on her behalf.
Alice decides to change the allowance to 200 tokens.
Bob, an attacker, monitors the blockchain and sees Alice's transaction to change the allowance.
Bob sends a transaction to spend the 100 tokens (the current allowance) immediately after Alice's transaction is sent, but before it is mined.

The transactions are mined in the following sequence:

Bob's transaction to spend 100 tokens.
Alice's transaction to set the allowance to 200 tokens.
Bob successfully spends 100 tokens, and Alice's allowance changes to 200, leading to an unintended token transfer of 100 tokens to Bob.

Attachments

Proof of Concept (PoC) File

    function compound() external {
        IERC20 fenixCache = IERC20(fenix);
        uint256 currentBalance = fenixCache.balanceOf(address(this));
        if (currentBalance > 0) {
            address votingEscrowCache = votingEscrow;
            fenixCache.safeApprove(votingEscrowCache, currentBalance);
            IVotingEscrowV1_2(votingEscrowCache).deposit_for_without_boost(managedTokenId, currentBalance);
            ISingelTokenVirtualRewarder(virtualRewarder).notifyRewardAmount(currentBalance);
            emit Compound(msg.sender, currentBalance);
        }
    }

Recomendation:

Always do safeApprove(0) if the allowance is being changed, or use safeIncreaseAllowance().

hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d