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

9 stars 7 forks source link

DIV - [H-1] Problem with Automatic Restaking in Smart Contract #235

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

DIV

high

[H-1] Problem with Automatic Restaking in Smart Contract

Summary

The smart contract in question allows a user to exit early through earlyExitById with a penalty or exit late through exitLateById without a penalty. However, there is a potential issue that token are automatically restaked after the lockPeriod for the time that is minimum of defaultRelockTime and LockPeriod . So, if a user wish to withdraw his token through exitLateById they has to wait for the new duration .

Vulnerability Detail

The vulnerability lies in the late exit mechanism of the contract. If a user does not withdraw their tokens within the lockPeriod, the contract automatically restakes the tokens for the duration which is minimum of original lock period and default relock time. This automatic restaking could potentially lock the user's tokens for an unintended period of time. Also, the user cannot choose earlyExitById as this option will result in potential loss of their staking token.

Impact

The impact of this vulnerability is significant. Users who intend to withdraw their tokens after the staking period may find their tokens locked for an additional period. This could affect their liquidity and could potentially lead to loss of investment opportunities.

Code Snippet

pragma solidity 0.8.23;

import "forge-std/Test.sol";
import {Lock} from "../src/Lock.sol";
import {LockList} from "../src/libraries/LockList.sol";
import "forge-std/console.sol";
import {MockToken} from "../src/mock/MockToken.sol";
/// @title Deposit Tests for Lock
/// @dev This contract implements tests using Foundry's test framework to simulate various deposit scenarios and reward distributions.
contract DepositTest is Test {
Lock lock;
    LockList locklist;
    address deployer;
    address user1;
    address user2;
    address user3;
    address user4;
    address user5;
    MockToken _stakingToken;
    MockToken _rewardToken;

    uint256[] lockPeriod;
    uint256[] lockMultiplier;
    address[] rewardTokens;

    /// @notice Tests simple deposit functionality and correct reward allocation
    /// @dev Simulates three users making deposits under different lock types and checks correct reward distribution after rewards are added.
    function setUp() public {
        user1 = vm.addr(1);

        deployer = vm.addr(9999999);
        uint256 forkId = vm.createFork("https://eth-mainnet.g.alchemy.com/v2/VZH1EtzO3RVWYSXs523zfhFzlO6KHnr6");
        vm.selectFork(forkId);

        vm.startPrank(deployer);
        lock = new Lock();
        locklist = new LockList(deployer);
        // transfer ownership of locklist
        lock.initialize(address(locklist), 15000, 35000, deployer);
        _stakingToken = new MockToken("stakingToken", "stkToken", 18);
        _rewardToken = new MockToken("rewardToken", "reward", 18);
        rewardTokens.push(address(_rewardToken));

        lock.setStakingToken(address(_stakingToken));
        lock.addReward(address(_rewardToken));
        lockPeriod.push(30 days);
        lockPeriod.push(60 days);
        lockPeriod.push(360 days);
        lockMultiplier.push(1);
        lockMultiplier.push(1);
        lockMultiplier.push(1);
        lock.setLockTypeInfo(lockPeriod, lockMultiplier);
        _stakingToken.mint(user1, 1000e18);

        _rewardToken.mint(deployer, 10000e18);
        lock.setTreasury(address(0x4));
        locklist.transferOwnership(address(lock));
        vm.stopPrank();

        vm.prank(user1);
        _stakingToken.approve(address(lock), 10000e18);

    }
    function test_exitEarly() public {
        vm.prank(user1);
        lock.stake(100,user1,2);

        vm.prank(deployer);
        _rewardToken.transfer(address(lock), 1000);
        vm.prank(deployer);
        lock.notifyUnseenReward(rewardTokens);

        vm.warp(block.timestamp + 360 days);

        vm.prank(user1);
        lock.earlyExitById(3);

        vm.prank(user1);
        assert( _stakingToken.balanceOf(user1) == 10000e18);
        //999999999999999999850    // user gets the reduced fund 

    }

    function test_exitLate() public {
        vm.prank(user1);
        lock.stake(100,user1,2);

        vm.prank(deployer);
        _rewardToken.transfer(address(lock), 1000);
        vm.prank(deployer);
        lock.notifyUnseenReward(rewardTokens);

        vm.warp(block.timestamp + 360 days);

        vm.prank(user1);
        lock.exitLateById(3);

         vm.prank(user4);
       lock.withdrawUnlockedTokenById(3);

        vm.prank(user4);
        assert( _stakingToken.balanceOf(user4) == 10000e18);
        //999999999999999999700   user doesnt get his funds back ass 300 was deposited

    }

Tool used

Manual Review

Recommendation

It is recommended to add a mechanism that allows users to withdraw their tokens on time in the late exit scenario. One possible solution could be to add a function that allows users to set a withdrawal time. This would give users more control over their tokens and prevent unintended restaking.

santipu03 commented 4 months ago

The protocol is designed to automatically re-lock a position when it is expired, and that's why early exits of expired positions are forced to be re-locked for defaultRelockTime. This is the intended design of the protocol, and that's why this issue is invalid.

Divyanshu-Madhav commented 4 months ago

Escalate The intended design is that if stakers exit early ,they will be penalised ,but if they wait for the the lock Period ,the tokens will be relocked for the defaultRelockPeriod.So the issue ,if the staker wants to liquidate his token just on time ,he just can't do it on time ,without getting penalised ,its similar to the M-2 issue mentioned in the audit report ,why it is invalid.

sherlock-admin3 commented 4 months ago

Escalate The intended design is that if stakers exit early ,they will be penalised ,but if they wait for the the lock Period ,the tokens will be relocked for the defaultRelockPeriod.So the issue ,if the staker wants to liquidate his token just on time ,he just can't do it on time ,without getting penalised ,its similar to the M-2 issue mentioned in the audit report ,why it is invalid.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.