New staking positions still gets the full reward amount as with old stakings, diluting rewards for old stakers

Summary

New staking positions still gets the full reward amount as with old stakings, diluting rewards for old stakers. Furthermore, due to the way multipliers are calculated, extremely short stakings are still very effective in stealing long-term stakers' rewards.

Vulnerability Detail

In the Magic LUM Staking system, users can lock-stake MLUM in exchange for voting power, as well as a share of the protocol revenue. As per the docs:

You can stake and lock your Magic LUM token in the Magic LUM staking pools to benefit from protocol profits. All protocol returns like a part of the trading fees, Fairlaunch listing fees, and NFT trading fees flow into the staking pool in form of USDC.

Rewards are distributed every few days, and you can Claim at any time.

However, when rewards are distributed, new and old staking positions are treated alike, and immediately receive the same rewards as it is distributed.

Thus, anyone can stake MLUM for a short duration as rewards are distributed, and siphon away rewards from long-term stakers. Staking for a few days still gets almost the same multiplier as staking for a year, and the profitability can be calculated and timed by calculating the protocol's revenue using various offchain methods (e.g. watching the total trade volume in each time intervals).

Consider the following scenario:

Alice stakes 50 MLUM for a year.
Bob has 50 MLUM but hasn't staked.
Bob notices that there is a spike in trading activity, and the protocol is gaining a lot of trading volume in a short time (thereby gaining a lot of revenue).
Bob stakes 50 MLUM for 1 second.
As soon as the rewards are distributed, Bob can harvest his part immediately.

Note that expired positions, while should not be able to vote, still accrue rewards. Thus Bob can just leave the position there and withdraw whenever he wants to without watching the admin actions. A more sophisticated attack involves front-running the admin reward distribution to siphon rewards, then unstake right away.

PoC

Due to the way multipliers are calculated, 1-year lock positions are only at most 3 times stronger than a 1-second lock for the same amount of MLUM.

The following coded PoC shows the given scenario, where Bob is able to siphon 25% of the rewards away by staking for a duration of a single second and leave it there.

Paste the following test into MlumStaking.t.sol, and run it by forge test --match-test testPoCHarvestDilute -vv:

function testPoCHarvestDilute() public {
    _stakingToken.mint(ALICE, 100 ether);
    _stakingToken.mint(BOB, 100 ether);

    vm.startPrank(ALICE);
    _stakingToken.approve(address(_pool), 50 ether);
    _pool.createPosition(50 ether, 365 days);
    vm.stopPrank();

    vm.startPrank(BOB);
    _stakingToken.approve(address(_pool), 50 ether);
    _pool.createPosition(50 ether, 1);
    vm.stopPrank();

    _rewardToken.mint(address(_pool), 100 ether);

    skip(100); // Bob can stake anytime and then just wait

    vm.prank(ALICE);
    _pool.harvestPosition(1);
    vm.prank(BOB);
    _pool.harvestPosition(2);

    console.logUint(_rewardToken.balanceOf(ALICE));
    console.logUint(_rewardToken.balanceOf(BOB));
}

The test logs will be

Ran 1 test for test/MlumStaking.t.sol:MlumStakingTest
[PASS] testPoCHarvestDilute() (gas: 940195)
Logs:
  75000000000000000000
  25000000000000000000

i.e. Bob was able to siphon 25% of the rewards.

Impact

Staking rewards can be stolen from honest stakers.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L354

Tool used

Manual Review

Recommendation

When new positions are created, their position should be recorded, but their amount with multipliers should be summed up and queued until the next reward distribution.

When the admin distributes rewards, there should be a function that first updates the pool, then add the queued amounts into staking. That way, newly created positions can still vote, but they do not accrue rewards for the immediate following distribution (only the next one onwards).

One must note down the timestamp that the position was created (as well as the timestamp the rewards were last distributed), so that when the position unstakes, the contract knows whether to burn the unstaked shares from the queued shares pool or the active shares pool.

sherlock-admin2 commented 4 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/7

sherlock-admin2 commented 3 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-06-magicsea-judging

PUSH0 - New staking positions still gets the full reward amount as with old stakings, diluting rewards for old stakers #74