sherlock-audit / 2024-06-magicsea-judging

2 stars 0 forks source link

Sticky Hickory Hare - A malicoius unlockOperator can vote unlimited times during a voting epoch #712

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

Sticky Hickory Hare

Low/Info

A malicoius unlockOperator can vote unlimited times during a voting epoch

Summary

According to MlumStaking contract, _unlockOperators are addresses who are able to forcibly unlock their positions regardless of whether lockDuration has been passed or not. Its not clear what contracts/EOAs are going to have this role, this issue assumes that there might exist a malicious _unlockOperator

Vulnerability Detail

MlumStaking::withdrawFromPosition shows that if nftOwner is an _unlockOperator, the position can be unlocked regardless of whether position is unlocked or not: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L624-L628

    function _withdrawFromPosition(address nftOwner, uint256 tokenId, uint256 amountToWithdraw) internal {
        require(amountToWithdraw > 0, "null");
        // withdrawFromPosition: amount cannot be null

        StakingPosition storage position = _stakingPositions[tokenId];
        require(
            _unlockOperators.contains(nftOwner) //@audit if is an unlock operator, unlock
                || (position.startLockTime + position.lockDuration) <= _currentBlockTimestamp() || isUnlocked(),
            "locked"
        );
      //....

the malicious operator can withdraw his position using withdrawFromPosition, create a new position (createPosition) using received tokens and sufficient lockDuration and initialLockDuration to be eligible for casting votes (Voter::vote):

    function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
        //...
        if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
            revert IVoter__AlreadyVoted();
        }

        if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
            revert IVoter__InsufficientLockTime();
        }
        if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }
       //....
        }

as you can see in above function, new position meets all the conditions and is eligible for casting another vote on this epoch. malicious operator can repeat this process unlimited amount of times within same voting epoch: Voter::vote ==> MlumStaking::withdrawFromPosition => MlumStaking::createPosition ==> Voter::vote ==> ....

Impact

only pools that malicious operator decides will be set as top ones, however, since this operator can be removed by owner and since top pools are calculated using emitted events, the impacts of exploitation can be easily fixed.

Code Snippet

Tool used

Manual Review

Recommendation

unlock operators should not be eligible for casting votes