Attackers can restake unlocked tokens if stakingToken is an ERC777 token.

Summary

The withdrawUnlockedTokenById() function does not comply with check-effect-interact pattern. The transfer of staking token happens before removing the lockId from the locklist, and the attacker can conduct an reentrancy attack to perform a restaking. The attacker would be able to withdraw and restake the staking token at the same time.

Vulnerability Detail

First of all, the contest readme states that The tokens we expect to interact with would be standard ERC-20 tokens. The staking token will be a standard ERC-20 token. The reward tokens will also be standard ERC-20 tokens. And given ERC777 tokens also follow the standards of ERC20 tokens, so the protocol should handle security issues where the staking token is an ERC777 token.

The vulnerability lies in withdrawUnlockedTokenById(), where the stakingToken transfer happens before removing the lock from locklist. A reentrancy attack can be performed upon token transfer to call restakeAfterLateExit() to create a restake position, while also withdrawing the token.

    function restakeAfterLateExit(uint256 id, uint256 typeIndex) external {
        // Retrieve the lock details for the specified ID.
>       LockedBalance memory lockedBalance = locklist.getLockById(msg.sender, id);
        require(lockedBalance.exitedLate, "This lock was not exited late or is ineligible for restaking.");

        uint256 newLockPeriod = lockPeriod[typeIndex]; // Get the new lock period based on the type index.
        uint256 currentLockPeriod = lockedBalance.lockPeriod;

        // Enforce that the new lock period must be valid based on the current conditions.
        if (currentLockPeriod <= defaultRelockTime || (block.timestamp - lockedBalance.lockTime) < currentLockPeriod) {
            require(newLockPeriod >= currentLockPeriod, "New lock period must be greater than or equal to the current lock period");
        } else {
            require(newLockPeriod >= defaultRelockTime, "New lock period must be greater than or equal to the default relock time");
        }

        // Proceed to restake the funds using the new lock type.
 >      _stake(lockedBalance.amount, msg.sender, typeIndex, true);

        // Remove the old lock record to prevent any further operations on it.
        locklist.removeFromList(msg.sender, id);

        emit RestakedAfterLateExit(msg.sender, id, lockedBalance.amount, typeIndex);
    }

    function withdrawUnlockedTokenById(uint256 id) external nonReentrant {
        LockedBalance memory lockedBal = locklist.getLockById(msg.sender, id); // Retrieves the lock details for the specified ID.
        if (lockedBal.unlockTime != 0 && lockedBal.unlockTime < block.timestamp) {
>           IERC20(stakingToken).safeTransfer(msg.sender, lockedBal.amount); // Transfers the unlocked amount to the user.
>           locklist.removeFromList(msg.sender, id); // Removes the lock from the lock list.
            emit WithdrawUnlockedById(id, msg.sender, lockedBal.amount); // Emits an event logging the withdrawal of the unlocked tokens.
        }
    }

Impact

Attackers can use reentrancy attack to restake and withdraw the staking token at the same time.

Code Snippet

https://github.com/sherlock-audit/2024-05-gamma-staking/blob/main/StakingV2/src/Lock.sol#L437-L444

Tool used

Manual review

Recommendation

Move the line locklist.removeFromList(msg.sender, id); before transferring the staking token in function withdrawUnlockedTokenById().

sherlock-audit / 2024-05-gamma-staking-judging

pkqs90 - Attackers can restake unlocked tokens if stakingToken is an ERC777 token. #241