csanuragjain - User might lose funds on extending lock

[sherlock-admin]

csanuragjain

high

User might lose funds on extending lock

Summary

While extending lock, if the resulting mintAmount becomes lesser than userDeposit.shareAmount (due to varying Multiplier curve) then user own deposit are burned which means user loses funds for extending duration which is wrong

Vulnerability Detail

1. User had a deposit of amount 100 for duration X which gave him mint Amount of 50
2. After some time, User wants to extend duration by y ie new duration should be X+y using extendLock
3. Due to varying Multiplier curve, the extendLock function resulted in new mint Amount of 40 with the duration X+y
4. Since the new mint Amount of 40 is lesser than userDeposit.shareAmount so user deposit are burned which does not make sense since user is penalized for keeping his funds locked for more duration (instead he should be rewarded for locking amount)

function extendLock(uint256 _depositId, uint256 _increaseDuration) external {
else if (mintAmount < userDeposit.shareAmount) {
            depositsOf[_msgSender()][_depositId].shareAmount =  mintAmount;
            _burn(_msgSender(), userDeposit.shareAmount - mintAmount);
        }
}

Impact

User will lose there funds

Code Snippet

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockPool.sol#L176

Tool used

Manual Review

Recommendation

Revert if mintAmount < userDeposit.shareAmount so that user is prevented from loss. In this case it is advisable for user to start a new deposit

[jack-the-pug]

Invalid as this is based on a wrong understanding of the design:

User had a deposit of amount 100 for duration X which gave him mint Amount of 50
Due to varying Multiplier curve, the extendLock function resulted in new mint Amount of 40 with the duration X+y

The curve is designed to be monotonic increasing.