Denial-of-Service Attack caused by nonReentrant modifier
Denial-of-Service Attack caused by nonReentrant modifier can occur in smart contracts, especially those deployed on Ethereum or similar blockchain platforms. The nonReentrant modifier is typically used to prevent reentrancy attacks, where a contract's function is recursively called before the previous call completes, potentially leading to unexpected behavior or manipulation of the contract's state.
Details
The following functions contain the nonReentrant modifier:
- HighStreetPoolBase.stake()
- HighStreetPoolBase.unstake()
- HighStreetPoolBase.updateStakeLock()
- HighStreetPoolBase.processRewards()
- HighStreetCorePool.transferHighToken()
- HighStreetCorePool.transferHighTokenFrom()
When receiveVaultRewards() function is called the pendingVaultRewards of the stakers will be always different from 0. In this case, when HighStreetPoolBase.unstake() is called, the flow below will be executed:
1. HighStreetPoolBase.unstake() (nonReentrant)
2. - - HighStreetCorePool._unstake()
3. - - - - HighStreetPoolBase._unstake()
4. - - - - - - HighStreetPoolBase._sync()
5. - - - - - - HighStreetCorePool._processRewards()
6. - - - - - - - - HighStreetCorePool._processVaultRewards()
7. - - - - - - - - - - HighStreetCorePool.transferHighToken()(nonReentrant again)
In this case, HighStreetCorePool.transferHighToken() will be called by HighStreetPoolBase.unstake(), which is not possible as both functions got the nonReentrant modifier. This only happens when pendingVaultRewards > 0 as per the code below:
function _processVaultRewards ( address _staker ) private {
User storage user = users [ _staker ];
uint256 pendingVaultClaim = pendingVaultRewards ( _staker );
if ( pendingVaultClaim == 0) return ;
// read HIGH token balance of the pool via standard ERC20 interface
uint256 highBalance = IERC20 ( HIGH ). balanceOf ( address ( this ));
require ( highBalance >= pendingVaultClaim , " contract HIGH balance too low ");
// update ` poolTokenReserve ` only if this is a HIGH Core Pool
if ( poolToken == HIGH ) {
// protects against rounding errors
poolTokenReserve -= pendingVaultClaim > poolTokenReserve ?
poolTokenReserve : pendingVaultClaim ;
}
user.subVaultRewards = weightToReward(user.totalWeight, vaultRewardsPerWeight);
// transfer fails if pool HIGH balance is not enough - which is a desired behavior
transferHighToken ( _staker, pendingVaultClaim );
emit VaultRewardsClaimed(msg.sender, _staker, pendingVaultClaim);
}
This would cause every single call to HighStreetPoolBase.unstake() to revert with the following error message: ReentrancyGuard: reentrant call
Recommendation
It is recommended to remove the nonReentrant modifier from the HighStreetCorePool.transferHighToken() function.