MorpheusAI

Summary

The order of state changes and external calls in the function Distribution._withdraw may introduce a reentrancy vulnerability.

Vulnerability Details

The order of state changes and external calls in the function Distribution._withdraw may introduce a reentrancy vulnerability. External calls should generally be placed at the end of the function to prevent potential reentrancy attacks.

• Found in contracts/Distribution.sol:

  function _withdraw(address user_, uint256 poolId_, uint256 amount_, uint256 currentPoolRate_) private {

  Pool storage pool = pools[poolId_];

  PoolData storage poolData = poolsData[poolId_];

  UserData storage userData = usersData[user_][poolId_];

  ​

  uint256 deposited_ = userData.deposited;

  require(deposited_ > 0, "DS: user isn't staked");

  ​

  if (amount_ > deposited_) {

  amount_ = deposited_;

  }

  ​

  uint256 newDeposited_;

  if (pool.isPublic) {

  require(

  block.timestamp < pool.payoutStart ||

  (block.timestamp > pool.payoutStart + pool.withdrawLockPeriod &&

  block.timestamp > userData.lastStake + pool.withdrawLockPeriodAfterStake),

  "DS: pool withdraw is locked"

  );

  ​

  uint256 depositTokenContractBalance_ = IERC20(depositToken).balanceOf(address(this));

  if (amount_ > depositTokenContractBalance_) {

  amount_ = depositTokenContractBalance_;

  }

  ​

  newDeposited_ = deposited_ - amount_;

  ​

  require(amount_ > 0, "DS: nothing to withdraw");

  require(newDeposited_ >= pool.minimalStake || newDeposited_ == 0, "DS: invalid withdraw amount");

  } else {

  newDeposited_ = deposited_ - amount_;

  }

  ​

  uint256 pendingRewards_ = _getCurrentUserReward(currentPoolRate_, userData);

  ​

  // Update pool data

  poolData.lastUpdate = uint128(block.timestamp);

  poolData.rate = currentPoolRate_;

  poolData.totalDeposited -= amount_;

  ​

  // Update user data

  userData.rate = currentPoolRate_;

  userData.deposited = newDeposited_;

  userData.pendingRewards = pendingRewards_;

  ​

  if (pool.isPublic) {

  totalDepositedInPublicPools -= amount_;

  ​

  IERC20(depositToken).safeTransfer(user_, amount_);

  }

  emit UserWithdrawn(poolId_, user_, amount_);

  }

Impact

User and/or pool deposit may be drained.

Tools Used

Hardhat

Proof of Code (POC)

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract ReentrancyVulnerableToken is IERC20 {
mapping(address => uint256) private balances;

function transfer(address to, uint256 amount) external override returns (bool) {
    require(balances[msg.sender] >= amount, "Insufficient balance");
    balances[msg.sender] -= amount;
    balances[to] += amount;
    return true;
}

function balanceOf(address account) external view override returns (uint256) {
    return balances[account];
}

// Simplified for demonstration purposes
function safeTransfer(address to, uint256 amount) external {
    // Assume external call here
    (bool success, ) = to.call{value: 0, gas: gasleft()}("");
    require(success, "External call failed");
}

}

contract ReentrancyVulnerableContract {
IERC20 public token;

constructor(address tokenAddress) {
    token = IERC20(tokenAddress);
}

function maliciousWithdraw() external {
    // Malicious token contract reenters this contract during the transfer
    token.safeTransfer(address(this), 1);
}

function withdrawAndFail() external {
    uint256 balanceBefore = token.balanceOf(address(this));
    
    // Vulnerable code where external call is before state changes
    token.safeTransfer(msg.sender, 1);

    // Incorrectly expecting the balance to be zero
    require(token.balanceOf(address(this)) == 0, "Balance not updated");
}

}

Recommendations

To reduce the risk of reentrancy attacks, the line IERC20(depositToken).safeTransfer(user_, amount_); should be moved outside of the if (pool.isPublic) block and placed after the state changes.

By moving the safeTransfer call outside of the if (pool.isPublic) block, you ensure that the state changes are already completed before any external interaction takes place. This helps in minimizing the risk of reentrancy attacks.