bbl4de - createPosition() and addToPosition() are callable even in case of emergency, decreasing rewards for users

[sherlock-admin4]

bbl4de

Medium

createPosition() and addToPosition() are callable even in case of emergency, decreasing rewards for users

Summary

Any user can create MLUM staking position with createPosition() during emergency and add more funds to it with addToPosition(), assuming the lockDuration is set to 0 - and therefore the multiplier is 0.

Vulnerability Detail

Creation of new positions should not be allowed during an emergency according to the following comment in createPosition() function:

// no new lock can be set if the pool has been unlocked
        if (isUnlocked()) {
            require(lockDuration == 0, "locks disabled");
        }

Unfortunately, the check in the code snippet above still allows locks with duration set to 0. This kind of call will not fail, as according to the sponsor, creation of locks with lockDuration == 0 is an intended functionality - it simply creates a staking position with no multiplier as in getMultiplierByLockDuration():

if (_maxLockDuration == 0 || lockDuration == 0) return 0;

However, in case of an emergency ANY call to createPosition() AND addToPosition() must be reverted.

Impact

Creating a position during emergency would decrease the rewards that other users are eligible for. As the users are allowed to choose to withdraw their funds during emergency with withdrawFromPosition(), rather than emergencyWithdraw(). Any user withdrawing their funds without the rewards - through emergencyWithdraw() would incentivize the malicious user to call addToPosition() and increase their stake to gain more rewards when they finally withdraw they stake. Note that the malicious user can withdraw their funds at any time, because their lockDuration was set to 0.

Proof of Code

A test file that is deploying locally all the required contracts from the codebase was created to provide proof of codes for MagicSea audit. In the ./magicsea-staking/test directory create a PoC.t.sol test file and paste into it the code from this gist. For the code to compile the name of the ERC20Mock contract in ./src/mocks/ERC20Mock.sol needs to be changed to ERC20MockWithRoles. Then, run:

forge test --mt test_createPosition_and_addToPosition_callable_during_emergency

For simplicity of the test the rewards where not included, but it is verified that the position is created successfully and _stakedSupplyWithMultiplier is increased. We can see in _updatePool() function ( called before any reward harvesting ) that _accRewardsPerShare is updated based on _stakedSupplyWithMultiplier :

_accRewardsPerShare =
            accRewardsPerShare +
            ((accruedReward * (PRECISION_FACTOR)) /
                (_stakedSupplyWithMultiplier));

Therefore, rewards distribution would be indeed affected.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L356

Tool used

Manual Review

Recommendation

The essential part of the mitigation is locking the createPosition() and addToPosition() functions completely if the contract is in emergency by adding the following check:

require(!isUnlocked(), "locks disabled");

similarily to the _lockPosition() function where it's implemented correctly.

Although it does not present an attack vector in itself, allowing staking positions with lockDuration set to 0 should be reconsidered due to its questionable fairness and lack of risk for stakers who choose this option ( no lock on their funds ).

[WangSecurity]

Invalid based on #290 (comment) and #290 (comment) comments.