sheep - Lack of parameter in MlumStaking::renewLockPosition leads to lack of change in IMlumStaking::StakingPosition which results in loss of rewards for stakers

[sherlock-admin2]

sheep

Medium

Lack of parameter in MlumStaking::renewLockPosition leads to lack of change in IMlumStaking::StakingPosition which results in loss of rewards for stakers

Summary

Wrong argument to a call to internal function MlumStaking::_lockPosition through MlumStaking::renewLockPosition leads to loss of rewards.

Vulnerability Detail

Staker creates a position in the protocol but has the ability to renew the IMlumStaking::StakingPosition by using the function MlumStaking::renewLockPosition, this function uses an internal function _lockPositon to renew the StakingPosition . During the call to this internal function _lockPosition(tokenId, _stakingPositions[tokenId].lockDuration, false); it passes the wrong second argument, in which instead of passing new lockDuration, it passes the same old lockDuration of staker. This keeps the status of stakingPosition the same despite renewing it. This could lead to a loss of rewards for the staker as rewards for staking positions depend on the duration for which the assets are staked in the protocol.

Impact

Loss of rewards for stakers

Code Snippet

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

Proof of Concept

function testDoesNotRenewLockDuration() public {
        _stakingToken.mint(ALICE, 2 ether);

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

        MlumStaking.StakingPosition memory position = _pool.getStakingPosition(1);
        uint256 lockDurationBeforeRenewLockPositionFunction = position.lockDuration;

        vm.startPrank(ALICE);
        _pool.renewLockPosition(1);
        vm.stopPrank();

        uint256 lockDurationAfterRenewLockPositionFunction = position.lockDuration;

        //Lock duration should have been different but this assertion passes
        assert(lockDurationAfterRenewLockPositionFunction == lockDurationBeforeRenewLockPositionFunction);
    }

function testDoesNotChangeTheRewardsForTheStakerIfHeRenewsTheLockDuration() public {
        _stakingToken.mint(ALICE, 2 ether);
        _stakingToken.mint(BOB, 2 ether);

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

        skip(2 days);
        vm.prank(ALICE);
        _pool.withdrawFromPosition(1, 1 ether);
        

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

        //We try to renew the LockPosition
        vm.startPrank(BOB);
        _pool.renewLockPosition(2);
        vm.stopPrank();

        skip(2 days);
        vm.prank(BOB);
        _pool.withdrawFromPosition(2, 1 ether);

        /**As the postion for BOB has been renewed this assertion should have failed as rewards for both stakers 
         * should have been different but it passes indicating the loss of rewards for stakers*/
        assert(_stakingToken.balanceOf(BOB) == _stakingToken.balanceOf(ALICE));



    }

Tool used

Manual Review

Recommendation

-  function renewLockPosition(uint256 tokenId) external nonReentrant {
+ function renewLockPosition(uint256 tokenId, uint256 lockDuration) external nonReentrant {
         _requireOnlyApprovedOrOwnerOf(tokenId);

         _updatePool();
-       _lockPosition(tokenId, _stakingPositions[tokenId].lockDuration, false);
+      _lockPosition(tokenId, lockDuration, false);
 }

Duplicate of #138