Open sherlock-admin2 opened 1 month ago
This issue causes users to keep the same multiplier by reducing lock times, thus gaining an advantage.
This creates an unfair advantage in the system and undermines the reliability of the staking mechanism.
PR: https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/138 Fixes scenario 2.
For scenario 1: it is a design choice that the amount mutliplier stays the same even lock duration ended.
The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/5
The Lead Senior Watson signed off on the fix.
robertodf
Medium
MlumStaking::addToPosition
should assing the amount multiplier based on the new lock duration instead of initial lock duration.Summary
There are two separate issues that make necessary to assign the multiplier based on the new lock duration:
First, when users add tokens to their position via
MlumStaking::addToPosition
, the new remaining time for the lock duration is recalculated as the amount-weighted lock duration. However, when the remaining time for an existing deposit is 0, this term is omitted, allowing users to retain the same amount multiplier with a reduced lock time. Consider the following sequence of actions:Alice creates a position by calling
MlumStaking::createPosition
depositing 1 ether and a lock time of 365 daysAfter the 365 days elapse, Alice adds another 1 ether to her position. The snippet below illustrates how the new lock time for the position is calculated:
The result will be:
(0*1 ether + 1 ether*365 days)/ 2 ether
, therefore Alice will need to wait just half a year, while the multiplier remains unchanged.Second, the missalignment between this function and
MlumStaking::renewLockPosition
creates an arbitrage opportunity for users, allowing them to reassign the lock multiplier to the initial duration if it is more beneficial. Consider the following scenario:MlumStaking::renewLockPosition
. The new amount multiplier will be calculated as ≈1+90/365*2 < 3
.Vulnerability Detail
Impact
You may find below the coded PoC corresponding to each of the aforementioned scenarios:
See PoC for scenario 1
Place in `MlumStaking.t.sol`. ```javascript function testLockDurationReduced() public { _stakingToken.mint(ALICE, 2 ether); vm.startPrank(ALICE); _stakingToken.approve(address(_pool), 1 ether); _pool.createPosition(1 ether, 365 days); vm.stopPrank(); // check lockduration MlumStaking.StakingPosition memory position = _pool.getStakingPosition( 1 ); assertEq(position.lockDuration, 365 days); skip(365 days); // add to position should take calc. avg. lock duration vm.startPrank(ALICE); _stakingToken.approve(address(_pool), 1 ether); _pool.addToPosition(1, 1 ether); vm.stopPrank(); position = _pool.getStakingPosition(1); assertEq(position.lockDuration, 365 days / 2); assertEq(position.amountWithMultiplier, 2 ether * 3); } ```See PoC for scenario 2
Place in `MlumStaking.t.sol`. ```javascript function testExtendLockAndAdd() public { _stakingToken.mint(ALICE, 2 ether); vm.startPrank(ALICE); _stakingToken.approve(address(_pool), 2 ether); _pool.createPosition(1 ether, 365 days); vm.stopPrank(); // check lockduration MlumStaking.StakingPosition memory position = _pool.getStakingPosition( 1 ); assertEq(position.lockDuration, 365 days); skip(365 days - 90 days); vm.startPrank(ALICE); _pool.addToPosition(1, 1 wei); // lock duration ≈ 90 days skip(30 days); _pool.renewLockPosition(1); // multiplier ≈ 1+90/365*2 position = _pool.getStakingPosition(1); assertEq(position.lockDuration, 7776000); assertEq(position.amountWithMultiplier, 1493100000000000001); _pool.addToPosition(1, 1 wei); // multiplier = 3 position = _pool.getStakingPosition(1); assertEq(position.lockDuration, 7776000); assertEq(position.amountWithMultiplier, 3000000000000000006); } ```Code Snippet
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L409-L417
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L509-L514
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L714
Tool used
Manual Review
Recommendation
Assign new multiplier in
MlumStaking::addToPosition
based on lock duration rather than initial lock duration.