Incorrect Cycle Adjustment in Staking Contract Withdrawal Function Results in Improper Fund Handling

[hats-bug-reporter[bot]]

Github username: @@BradMoonUESTC Twitter username: @xy9301 Submission hash (on-chain): 0xd1af652b2486fbfe2e59ba4d21cefce3445d7080e3e393b8fd74a01d6d246265 Severity: high

Description: In https://github.com/Cvg-Finance/hats-audit/blob/b3c66b1323cd555f5fa784b12736fd9b8f9ddfc5/contracts/Staking/Convex/StakingServiceBase.sol#L636-L699

Description The vulnerability exists in the _updateAmountStakedWithdraw function of a staking smart contract, specifically within the logic that handles withdrawal requests exceeding the pending amount for the next staking cycle. The function incorrectly modifies the staking totals for the current cycle instead of the intended next cycle. This logic flaw leads to incorrect decrementing of the totalStaked and amountStaked values for the current cycle when the operations should only affect the subsequent cycle. Such discrepancies can result in incorrect token balances and potentially manipulate the reward distributions, undermining the integrity of the staking mechanism.

Attack Scenario An attacker can exploit this vulnerability by first ensuring that there are tokens staked for the upcoming cycle, creating a scenario where nextCyclePending is greater than zero. They then initiate a withdrawal amount that exceeds this pending amount. Due to the erroneous code logic, this action improperly decreases the totalStaked and amountStaked for the current cycle rather than adjusting the next cycle's values. This could allow an attacker to withdraw more funds than they should be entitled to, impacting other stakers by reducing the total staked amount reported for the current cycle, potentially leading to incorrect reward calculations and distributions.

Attachments

1. Proof of Concept (PoC) File

   • This file would typically contain smart contract interactions that simulate the deposit and withdrawal actions described in the attack scenario, demonstrating the fund mismanagement due to the vulnerability.

2. Revised Code File (Optional)

   • A potential fix for this vulnerability can be applied by ensuring that the adjustments to totalStaked and amountStaked are made on the correct cycle's records. The provided revised code would include comments explaining the changes and ensuring that any adjustments made after handling nextCyclePending amounts are directed at the nextCycle records, not the current cycle.

// Revised _updateAmountStakedWithdraw function
function _updateAmountStakedWithdraw(uint256 tokenId, uint256 amount, uint256 currentCycle) internal {
    uint256 nextCycle = currentCycle + 1;
    uint256 nextCyclePending = _tokenInfoByCycle[nextCycle][tokenId].pendingStaked;
    uint256 _tokenTotalStaked = tokenTotalStaked(tokenId);

    require(amount <= _tokenTotalStaked, "WITHDRAW_EXCEEDS_STAKED_AMOUNT");
    uint256 _newTokenStakedAmount = _tokenTotalStaked - amount;

    uint256 _lastActionCycle = _stakingHistoryByToken[tokenId][_stakingHistoryByToken[tokenId].length - 1];
    uint256 _lastStakedAmount = _lastActionCycle < currentCycle ? 
                                _tokenInfoByCycle[_lastActionCycle][tokenId].amountStaked : 
                                _tokenInfoByCycle[currentCycle][tokenId].amountStaked;

    if (nextCyclePending != 0) {
        if (nextCyclePending >= amount) {
            _tokenInfoByCycle[nextCycle][tokenId].pendingStaked -= amount;
        } else {
            amount -= nextCyclePending;
            delete _tokenInfoByCycle[nextCycle][tokenId].pendingStaked;
            // Adjust the correct cycle's total staked and amount staked
            _cycleInfo[nextCycle].totalStaked -= amount; // Corrected to nextCycle
            _tokenInfoByCycle[nextCycle][tokenId].amountStaked = _lastStakedAmount - amount; // Corrected to nextCycle
        }
    } else {
        _cycleInfo[nextCycle].totalStaked -= amount; // Adjustments should reflect on nextCycle even if no pending amount
        _tokenInfoByCycle[nextCycle][tokenId].amountStaked = _lastStakedAmount - amount;
    }
}

This revised code snippet includes clear comments indicating the modifications intended to correct the identified vulnerability. The changes ensure that any adjustments to the staked amounts are made to the correct cycle's records, maintaining the integrity of the contract's state and preventing potential exploitation.

[PlamenTSV]

The next cycle is correctly updated before the pending stake logic. Read the comments of the function first to understand how it works. If we try to withdraw more than we have staked on the next cycle, we take out stake from the current one to cover the withdrawal. This is why we reduce the currentCycle stake and total stake.