MorpheusAI

Summary

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

Vulnerability Details

The order of state changes and external calls in the function Distribution._stake 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 _stake(address user_, uint256 poolId_, uint256 amount_, uint256 currentPoolRate_) private {

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

  ​

  Pool storage pool = pools[poolId_];

  PoolData storage poolData = poolsData[poolId_];

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

  ​

  if (pool.isPublic) {

  // https://docs.lido.fi/guides/lido-tokens-integration-guide/#steth-internals-share-mechanics

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

  //@audit: potential H-issue - anyone can transfer tokens from the `from` address if an approval is made?

  //@audit: reentrancy risk - the state is updated after the external call. The line below should be placed after the update of user data & pool data

  IERC20(depositToken).safeTransferFrom(_msgSender(), address(this), amount_);

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

  ​

  amount_ = balanceAfter_ - balanceBefore_;

  ​

  require(userData.deposited + amount_ >= pool.minimalStake, "DS: amount too low");

  ​

  totalDepositedInPublicPools += amount_;

  }

  ​

  userData.pendingRewards = _getCurrentUserReward(currentPoolRate_, userData);

  //audit: is this a reentrancy issue since the state variable is written after the call?

  // Update pool data

  poolData.lastUpdate = uint128(block.timestamp);

  poolData.rate = currentPoolRate_;

  poolData.totalDeposited += amount_;

  ​

  // Update user data

  userData.lastStake = uint128(block.timestamp);

  userData.rate = currentPoolRate_;

  userData.deposited += amount_;

  ​

  emit UserStaked(poolId_, user_, amount_);

  }

Impact

Potential loss of funds.

Tools Used

Hardhat

Recommendations

To reduce the risk of reentrancy attacks, the lines
"IERC20(depositToken).safeTransferFrom(msgSender(), address(this), amount);" and
"userData.pendingRewards = getCurrentUserReward(currentPoolRate, userData);
"
should be moved outside of the if (pool.isPublic) block and placed after the state changes.

By moving these two lines of code 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.