Irissme - Lack of Transaction Guard in topUp Function

[sherlock-admin2]

Irissme

medium

Lack of Transaction Guard in topUp Function

Summary

The topUp function in the StakingRewardsManager.sol file lacks proper transaction guarding before invoking the notifyRewardAmount function. This could expose the contract to reentrancy attacks.

Vulnerability Detail

The topUp function first sets the rewards duration using staking.setRewardsDuration(config.rewardsDuration) and then transfers tokens to the staking contract using rewardToken.transferFrom. However, there is a potential vulnerability as the notifyRewardAmount function is called afterward without proper transaction guarding.

Impact

Without transaction guarding, there is a risk of reentrancy attacks, where an attacker could potentially manipulate the state of the contract during the execution of the notifyRewardAmount function.

Impact on a Hypothetical Scenario

Scenario Description:
The StakingRewardsManager contract manages multiple staking contracts.
An attacker exploits the lack of transaction guarding in the topUp function.

Exploitation Steps:

The attacker initiates a topUp transaction with a malicious staking contract index.
The attacker reverts the staking.setRewardsDuration(config.rewardsDuration) by triggering a reentrancy attack, manipulating the state.
The attacker drains the staking contract's funds during the execution of notifyRewardAmount.

Consequences:

Funds Manipulation: The attacker can manipulate the staking contract's state during the setRewardsDuration call, potentially draining its funds or causing other unintended behaviors.

Reentrancy Exploitation: Exploiting the lack of transaction guarding allows the attacker to repeatedly enter the staking contract and execute malicious actions.

Potential Damage:
Financial Loss: The attacker may drain funds from the staking contracts, causing financial losses to users.
Disruption of Staking: Continuous reentrancy attacks could disrupt the normal operation of the staking contracts, affecting legitimate stakers.
Mitigation:
Implementing proper transaction guarding, such as using the ReentrancyGuard modifier, would prevent the attacker from manipulating the state of the staking contracts during the execution of the notifyRewardAmount function. This would enhance the security of the topUp function and prevent the described attack scenario.

Conclusion:
The identified lack of transaction guarding in the topUp function poses a significant risk of financial loss and disruption to the staking contracts managed by the StakingRewardsManager. Implementing the recommended mitigation is crucial to prevent potential exploits and ensure the security of the contract.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L254-L277

Tool used

Manual Review

Recommendation

// Import the ReentrancyGuard from OpenZeppelin
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract StakingRewardsManager is AccessControlUpgradeable, ReentrancyGuard {
    // ... (existing contract code)

    // Add ReentrancyGuard to the contract
    modifier nonReentrant() {
        require(!inTopUp, "ReentrancyGuard: reentrant call");
        inTopUp = true;
        _;
        inTopUp = false;
    }

    // ... (existing contract code)

    function topUp(address source, uint256[] memory indices) external onlyRole(EXECUTOR_ROLE) nonReentrant {
        for (uint i = 0; i < indices.length; i++) {
            StakingRewards staking = stakingContracts[indices[i]];
            StakingConfig memory config = stakingConfigs[staking];

            // Secure against reentrancy attacks using ReentrancyGuard
            staking.setRewardsDuration(config.rewardsDuration);

            // Transfer tokens from the owner of this contract to fund the staking contract
            rewardToken.transferFrom(source, address(staking), config.rewardAmount);

            // No longer vulnerable to reentrancy attacks due to the ReentrancyGuard modifier
            staking.notifyRewardAmount(config.rewardAmount);

            emit ToppedUp(staking, config);
        }
    }
}

In the modified code, I added the ReentrancyGuard modifier to the contract and applied the nonReentrant modifier to the topUp function. This modification ensures that the function cannot be reentered until the previous execution is completed, mitigating the risk of reentrancy attacks.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { Its invalid }

[nevillehuang]

Invalid, topUp() is an admin permissioned function where admins are trusted to call this function appropriately. Additionally, there is insufficient proof that reentrancy is possible