sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

Sticky Hickory Hare - First Mlum staker can withdraw all the MlumStaking reward tokens if there are any #709

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

Sticky Hickory Hare

Low/Info

First Mlum staker can withdraw all the MlumStaking reward tokens if there are any

Summary

If some amount of reward tokens are going to be added to MlumStaking pool right after its deployment, first staker can use this opportunity to withdraw all the reward tokens from the contract as all the _stakedSupplyWithMultiplier belongs to himself.

Vulnerability Detail

Lets assume that MlumStaking is in its initial state and some amount of usdc (100 usdc) is added into the contract before first position is created. upon creating a new position, _updatePool is called: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L360

    function createPosition(
        uint256 amount,
        uint256 lockDuration
    ) external override nonReentrant {
        if (isUnlocked()) {
            require(lockDuration == 0, "locks disabled");
        }
        _updatePool();

for the first position, since _stakedSupply is 0, the function simply returns and doesn't do anything: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L397

        if (lastRewardBalance == rewardBalance || _stakedSupply == 0) {
            return;
        }

on the next call to createPosition, since _stakedSupply is greater than 0 and all _stakedSupplyWithMultiplier belongs to the first staker all the rewards will be allocated to it :

        uint256 accruedReward = rewardBalance - lastRewardBalance;
        _accRewardsPerShare =
            accRewardsPerShare +
            ((accruedReward * (PRECISION_FACTOR))
                (_stakedSupplyWithMultiplier));

first staker then simply calls harvestPosition(first_position_id) and withdraws all the 100 usdc from the contract. this simple test (add to MlumStaking.t.sol) demonstrates this issue:

    function testGetAllRewards() public {
        _rewardToken.mint(address(_pool), 100_000_000);
        _stakingToken.mint(ALICE, 2 ether);
        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), 2 ether);
        _pool.createPosition(1, 1 days); //_stakedSupply is zero, dont update pool
        _pool.createPosition(1 ether, 1 days); //distribute all 100_000_000 rewards to previous supply
        _pool.harvestPosition(1); //harvest position and receive all rewards
        vm.stopPrank();
        assertEq(_rewardToken.balanceOf(address(_pool)), 0);
        assertEq(_rewardToken.balanceOf(address(ALICE)), 100_000_000);
    }

Impact

In my opinion, impact of this issue could be anywhere from low to high depending on how team is going to allocate rewards to Mlum stakers, if a large amount of usdc is added into the pool before the first position, the damage will be higher.

Code Snippet

Tool used

Manual Review

Recommendation

amount of reward tokens going into MlumStaking contract should be proportional to amount of mlum tokens staked, This ensures that no large amount of reward tokens is available for exploitation by the first staker.