Reentrancy Vulnerability on Token Transfers

Summary

The OperationalStaking contract is potentially vulnerable to reentrancy attacks due to the external calls to the ERC20 token contract without proper reentrancy guards.

Vulnerability Detail

In several functions of the OperationalStaking contract, such as _transferToContract, _transferFromContract, redeemRewards, and redeemCommission, the contract interacts with the external ERC20 token contract. These interactions can potentially be exploited by a malicious contract to re-enter the staking contract before the first execution is completed, leading to unexpected behaviors or state corruption.

Impact

Reentrancy attacks can lead to loss of funds, double spending, or corrupted contract states. This vulnerability is particularly critical in financial contracts like staking mechanisms, where the integrity of fund management and accounting is paramount.

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L372-L381 https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L637 https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L637

Tool used

Manual Review

Recommendation

To mitigate reentrancy attacks, consider implementing the Checks-Effects-Interactions pattern and use a reentrancy guard. The OpenZeppelin library provides a ReentrancyGuard contract that can be used to prevent reentrancy. Here's how you could integrate it:

Import ReentrancyGuard from the OpenZeppelin library and inherit it in your contract.
Use the nonReentrant modifier on functions that make external calls to other contracts.

Here's an example of how you might apply these changes:

import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

contract OperationalStaking is OwnableUpgradeable, ReentrancyGuardUpgradeable {
    // Existing code...

    function initialize(address cqt, uint128 dCoolDown, uint128 vCoolDown, uint128 maxCapM, uint128 vMaxStake) external initializer {
        __Ownable_init();
        __ReentrancyGuard_init();
        // Rest of the initialization code...
    }

    function redeemRewards(uint128 validatorId, address beneficiary, uint128 amount) external nonReentrant {
        // Function logic...
    }

    function redeemCommission(uint128 validatorId, address beneficiary, uint128 amount) public nonReentrant {
        // Function logic...
    }
}

By applying the nonReentrant modifier, you ensure that no reentrancy is possible in critical functions, mitigating the risk of such attacks.

sherlock-admin2 commented 8 months ago

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

takarez commented:

invalid

nevillehuang commented 8 months ago

Invalid, you cannot reenter a standard ERC20

sherlock-audit / 2023-11-covalent-judging

Anubis - Reentrancy Vulnerability on Token Transfers #19