sherlock-admin3 commented 3 months ago

cocacola

High

Voting does not take into account end of staking lock period

Summary

The protocol allows to vote in Voter contract by means of staked position in MlumStaking. To vote, user must have staking position with certain properties. However, the voting does not implement check against invariant that the remaining lock period needs to be longer then the epoch time to be eligible for voting. Thus, it is possible to vote with stale voting position. Additionally, if position's lock period finishes inside of the voting epoch it is possible to vote, withdraw staked position, stake and vote again in the same epoch. Thus, voting twice with the same stake amount is possible from time to time. Ultimately, the invariant that voting once with same balance is only allowed is broken as well. The voting will decide which pools receive LUM emissions and how much.

Vulnerability Detail

The documentation states that:

Who is allowed to vote Only valid Magic LUM Staking Position are allowed to vote. The overall lock needs to be longer then 90 days and the remaining lock period needs to be longer then the epoch time.

User who staked position in the MlumStaking contract gets NFT minted as a proof of stake with properties describing this stake. Then, user can use that staking position to vote for pools by means of vote() in Voter contract. The vote() functions checks if initialLockDuration is higher than _minimumLockTime and lockDuration is higher than _periodDuration to process further. However, it does not check whether the remaining lock period is longer than the epoch time. Thus, it is possible to vote with stale staking position. Also, current implementation makes renewLockPosition and extendLockPosition functions useless.

    function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
        if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();

        // check voting started
        if (!_votingStarted()) revert IVoter_VotingPeriodNotStarted();
        if (_votingEnded()) revert IVoter_VotingPeriodEnded();

        // check ownership of tokenId
        if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
            revert IVoter__NotOwner();
        }

        uint256 currentPeriodId = _currentVotingPeriodId;
        // check if alreay voted
        if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
            revert IVoter__AlreadyVoted();
        }

        // check if _minimumLockTime >= initialLockDuration and it is locked
        if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
            revert IVoter__InsufficientLockTime();
        }
        if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }

        uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;

        // check if deltaAmounts > votingPower
        uint256 totalUserVotes;
        for (uint256 i = 0; i < pools.length; ++i) {
            totalUserVotes += deltaAmounts[i];
        }

        if (totalUserVotes > votingPower) {
            revert IVoter__InsufficientVotingPower();
        }

        IVoterPoolValidator validator = _poolValidator;

        for (uint256 i = 0; i < pools.length; ++i) {
            address pool = pools[i];

            if (address(validator) != address(0) && !validator.isValid(pool)) {
                revert Voter__PoolNotVotable();
            }

            uint256 deltaAmount = deltaAmounts[i];

            _userVotes[tokenId][pool] += deltaAmount;
            _poolVotesPerPeriod[currentPeriodId][pool] += deltaAmount;

            if (_votes.contains(pool)) {
                _votes.set(pool, _votes.get(pool) + deltaAmount);
            } else {
                _votes.set(pool, deltaAmount);
            }

            _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount);
        }

        _totalVotes += totalUserVotes;

        _hasVotedInPeriod[currentPeriodId][tokenId] = true;

        emit Voted(tokenId, currentPeriodId, pools, deltaAmounts);
    }

The documentation states that minimum lock period for staking to be eligible for voting is 90 days. The documentation states that voting for pools occurs biweekly.

Thus, assuming the implementation with configuration presented in the documentation, every 90 days it is possible to vote twice within the same voting epoch by:

voting,
withdrawing staked amount,
creating new position with staking token,
voting again.

    function createPosition(uint256 amount, uint256 lockDuration) external override nonReentrant {
        // no new lock can be set if the pool has been unlocked
        if (isUnlocked()) {
            require(lockDuration == 0, "locks disabled");
        }

        _updatePool();

        // handle tokens with transfer tax
        amount = _transferSupportingFeeOnTransfer(stakedToken, msg.sender, amount);
        require(amount != 0, "zero amount"); // createPosition: amount cannot be null

        // mint NFT position token
        uint256 currentTokenId = _mintNextTokenId(msg.sender);

        // calculate bonuses
        uint256 lockMultiplier = getMultiplierByLockDuration(lockDuration);
        uint256 amountWithMultiplier = amount * (lockMultiplier + 1e4) / 1e4;

        // create position
        _stakingPositions[currentTokenId] = StakingPosition({
            initialLockDuration: lockDuration,
            amount: amount,
            rewardDebt: amountWithMultiplier * (_accRewardsPerShare) / (PRECISION_FACTOR),
            lockDuration: lockDuration,
            startLockTime: _currentBlockTimestamp(),
            lockMultiplier: lockMultiplier,
            amountWithMultiplier: amountWithMultiplier,
            totalMultiplier: lockMultiplier
        });

        // update total lp supply
        _stakedSupply = _stakedSupply + amount;
        _stakedSupplyWithMultiplier = _stakedSupplyWithMultiplier + amountWithMultiplier;

        emit CreatePosition(currentTokenId, amount, lockDuration);
    }

Proof of Concept

Scenario 1:

function testGT_vote_twice_with_the_same_stake() public {
        vm.prank(DEV);
        _voter.updateMinimumLockTime(2 weeks);

        _stakingToken.mint(ALICE, 1 ether);

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

        skip(1 weeks);

        vm.prank(DEV);
        _voter.startNewVotingPeriod();

        vm.startPrank(ALICE);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        vm.expectRevert(IVoter.IVoter__AlreadyVoted.selector);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();

        assertEq(_voter.getTotalVotes(), 1 ether);

        skip(1 weeks + 1);

        vm.startPrank(ALICE);
        _pool.withdrawFromPosition(1, 1 ether);
        vm.stopPrank();

        vm.startPrank(ALICE);
        vm.expectRevert();
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 2 weeks);
        _voter.vote(2, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();

        assertEq(_voter.getTotalVotes(), 2 ether);
    }

Scenario 2:

function testGT_vote_twice_with_the_same_stake() public {
        vm.prank(DEV);
        _voter.updateMinimumLockTime(2 weeks);

        _stakingToken.mint(ALICE, 1 ether);

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

        skip(1 weeks);

        vm.prank(DEV);
        _voter.startNewVotingPeriod();

        vm.startPrank(ALICE);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        vm.expectRevert(IVoter.IVoter__AlreadyVoted.selector);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();

        assertEq(_voter.getTotalVotes(), 1 ether);

        skip(1 weeks + 1);

        vm.startPrank(ALICE);
        _pool.withdrawFromPosition(1, 1 ether);
        vm.stopPrank();

        vm.startPrank(ALICE);
        vm.expectRevert();
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 2 weeks);
        _voter.vote(2, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();

        assertEq(_voter.getTotalVotes(), 2 ether);
    }

Impact

A user can vote with stale staking position, then withdraw the staking position with any consequences. Additionally, a user can periodically vote twice with the same balance of staking tokens for the same pool to increase unfairly the chance of the pool being selected for further processing.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L172 https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L175

Tool used

Manual Review

Recommendation

It is recommended to enforce the invariant that the remaining lock period must be longer than the epoch time to be eligible for voting. Additionally, It is recommended to prevent double voting at any time. One of the solution can be to prevent voting within the epoch if staking position was not created before epoch started.

0xSmartContract commented 2 months ago

The vulnerability identified involves the voting mechanism within the Voter contract that allows users to vote using staked positions in the MlumStaking contract.

The issue arises because the contract does not validate if the remaining lock period of a staking position is longer than the voting epoch. As a result, users can vote with a staking position that has a lock period ending within the voting epoch, allowing potential double voting by.

This vulnerability can lead to:

Voting with stale staking positions.
Double voting within the same epoch, thus skewing the vote results unfairly.
Breaking the invariant that each stake amount should only vote once per epoch.

The vote function currently checks:

The ownership of the tokenId.
If the user has already voted in the current period.
The initial lock duration and current lock duration against minimum thresholds.

However, it fails to ensure that the remaining lock period exceeds the epoch time, allowing potential manipulation as described.

sherlock-admin2 commented 2 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/25

chinmay-farkya commented 2 months ago

Escalate

Some issues have been incorrectly duped with this issue :

219 is not a duplicate and is actually invalid as it involves a trusted admin to use a function at a wrong time. The assertion that the admin will start a new voting period when one is already running is completely unreasonable. Also, the readme clearly mentions that a keeper bot is meant to start new epochs periodically and its off-chain and out-of-scope.

306 is not a duplicate and is invalid as it ties to the same reasoning for #219 above

504 is again not a duplicate as it ties to the same reasoning for #219 above

405 is again not a duplicate as it ties to the same reasoning for #219 above

407 is not a duplicate and is invalid due to the same reason as #219, cited above.

252 is not a duplicate of this and is invalid/low because it just points out a difference between docs and code but it has no real impact or loss of funds. It talks about the ability to claim rewards later after more epoch periods have passed, which is completely normal and does not relate to a malicious action or a loss to the staker.

413 is invalid because frontrunning is not possible on iota

419 is a completely different issue and is a low because its an admin controlled action. Admin can wait for the current voting period to end, then update minimumLockTime and then start a new voting period. Since admin is trusted, they are expected to wait for current period to end and only checking time config starting from the next period.

From sherlock docs

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

There is a separate set of issues described as “expired positions can keep harvesting rewards”. This is not the same as what this issue 166 describes. Fixing 166 by not allowing voting using already expired or to-be-expired-soon during the voting period is not going to fix this set of issues, because harvesting rewards will still be possible in MLUMStaking.sol.

This set includes: #6, #530, #362 and #253

This set should be made a separate medium issue. It requires expired locks to be kicked or harvesting to be disabled for already expired locks.

sherlock-admin3 commented 2 months ago

Escalate

Some issues have been incorrectly duped with this issue :

219 is not a duplicate and is actually invalid as it involves a trusted admin to use a function at a wrong time. The assertion that the admin will start a new voting period when one is already running is completely unreasonable. Also, the readme clearly mentions that a keeper bot is meant to start new epochs periodically and its off-chain and out-of-scope.

306 is not a duplicate and is invalid as it ties to the same reasoning for #219 above

504 is again not a duplicate as it ties to the same reasoning for #219 above

405 is again not a duplicate as it ties to the same reasoning for #219 above

407 is not a duplicate and is invalid due to the same reason as #219, cited above.

252 is not a duplicate of this and is invalid/low because it just points out a difference between docs and code but it has no real impact or loss of funds. It talks about the ability to claim rewards later after more epoch periods have passed, which is completely normal and does not relate to a malicious action or a loss to the staker.

413 is invalid because frontrunning is not possible on iota

419 is a completely different issue and is a low because its an admin controlled action. Admin can wait for the current voting period to end, then update minimumLockTime and then start a new voting period. Since admin is trusted, they are expected to wait for current period to end and only checking time config starting from the next period.

From sherlock docs

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

There is a separate set of issues described as “expired positions can keep harvesting rewards”. This is not the same as what this issue 166 describes. Fixing 166 by not allowing voting using already expired or to-be-expired-soon during the voting period is not going to fix this set of issues, because harvesting rewards will still be possible in MLUMStaking.sol.

This set includes: #6, #530, #362 and #253

This set should be made a separate medium issue. It requires expired locks to be kicked or harvesting to be disabled for already expired locks.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0xSmartContract commented 2 months ago

219, #306, #504, #405, #407 , #419; This group is considered invalid under "Admin Input and responsibility", I agree.

252 low/Invalid , I agree (There is an escalation process in 252, so 252 should be evaluated as low/invalid.)

413 I do not agree.(The verdict is that front-running on IOTA is valid)

This issue was discussed and confirmed with Sherlock officials; The verdict is that front-running on IOTA is valid, but can be at most medium severity, since it's not reliable and uncontrollable by the attacker, though they can mitigate randomness with sending lot's of transactions. But later this year, this will be invalid, cause when IOTA launches version 2.0, they will have no mempool at all, while the current one is public.

6, #530, #362, #253 Set includes . I do not agree

0xf1b0 commented 2 months ago

413 is invalid because frontrunning is not possible on iota

Front running becomes possible when the reward schedule is known in advance. There's no necessity to monitor a pool and send transactions within the same block; this can be accomplished an hour earlier.

Although it may not be the most suitable title, the issue remains definitively valid.

J4X-98 commented 2 months ago

6, #530, #362, #253 Set includes . I do not agree

@0xSmartContract what do you mean by that?

Additionally regarding #413, tx are randomly ordered on the IOTA evm which makes frontrunning impossible. The IOTA EVM documentation also states this:

"To protect against Maximal Extractable Value attacks, the IOTA Smart Contract chain natively randomizes transaction ordering, making it impossible to determine the content of a block in order to manipulate it."

0xSmartContract commented 2 months ago

413 I do not agree.(The verdict is that front-running on IOTA is valid)

This issue was discussed and confirmed with Sherlock officials; The verdict is that front-running on IOTA is valid, but can be at most medium severity, since it's not reliable and uncontrollable by the attacker, though they can mitigate randomness with sending lot's of transactions. But later this year, this will be invalid, cause when IOTA launches version 2.0, they will have no mempool at all, while the current one is public.

Front-running on IOTA is valid I also said that there was no front-run like you, but Sherlock confirmed it , @WangSecurity ;

This issue was discussed and confirmed with Sherlock officials; The verdict is that front-running on IOTA is valid, but can be at most medium severity, since it's not reliable and uncontrollable by the attacker, though they can mitigate randomness with sending lot's of transactions. But later this year, this will be invalid, cause when IOTA launches version 2.0, they will have no mempool at all, while the current one is public.

J4X-98 commented 2 months ago

@0xSmartContract,

If the head of judging is of that opinion I guess we'll have to accept it. Although it makes almost no sense on a sandwiching attack, as the user would only be able to extract a minor part of the funds as he has to split his capital used for the sandwich into many txs.

Nevertheless could you tell me what you meant by the quoted part on top of my message?

0xSmartContract commented 2 months ago

@0xSmartContract,

If the head of judging is of that opinion I guess we'll have to accept it. Although it makes almost no sense on a sandwiching attack, as the user would only be able to extract a minor part of the funds as he has to split his capital used for the sandwich into many txs.

Nevertheless could you tell me what you meant by the quoted part on top of my message?

A set was mentioned in Escalate, I just wanted to say that this issue should not be separated into a different set.

J4X-98 commented 2 months ago

@0xSmartContract,

What's your reasoning for that? As mentioned by me on the escalation of #6 these are completely different issues.

0xSmartContract commented 2 months ago

@0xSmartContract,

What's your reasoning for that? As mentioned by me on the escalation of #6 these are completely different issues.

6 and #166

Both issues focus on gaining unfair advantage through positions whose lock period has expired.
Both issues deal with the ability of positions whose lock period has expired to remain valid in the current system.
Both issues propose that positions should be revoked if their lock period has expired to prevent this situation.

chinmay-farkya commented 2 months ago

I disagree with your take that these are the same issues. Just because they happen to be in the same component of functionality, they can't be duped.

One problem exists in the voter contract, and another exists in MLUMStaking. In 166, we need to check for remaining lock duration which can also expire during the period itself.

Thus the solution for 6 which is to kick expired positions is not going to solve 166 as positions that have not expired yet but soon-to-expire will still be able to take advantage in voting. They have separate problems and solutions, and are not dups.

Both issues focus on gaining unfair advantage through positions whose lock period has expired.

Not entirely true. 166 is also about soon-to-expire positions (ie. anytime in the next 14 days) and they can't be kicked.

Both issues deal with the ability of positions whose lock period has expired to remain valid in the current system.

Again not true due to the involvement of soon-to-expire positions

Both issues propose that positions should be revoked if their lock period has expired to prevent this situation.

Now that is completely false. 166 proposes to check remaining duration and not to "revoke positions" as some positions might still be active and would expire during the period itself, they can't be simply kicked.

J4X-98 commented 2 months ago

Both issues focus on gaining unfair advantage through positions whose lock period has expired.

This is a similar impact. But this is no reason for grouping issues. If you find 10 different ways to drain funds in a contract with different root causes you also can't merge them into one issue just because they all have the same impact.

Both issues deal with the ability of positions whose lock period has expired to remain valid in the current system.

This is a similar scenario, but still no reason for duping. That would be the same if you had multiple issues that occur while a contract is paused, but all originate from different root causes. You also can't duplicate those together.

Both issues propose that positions should be revoked if their lock period has expired to prevent this situation.

This is not true. #166 does not state anything about revoking the positions once they are expired. It just fixes the if statement.

WangSecurity commented 2 months ago

About the escalation comment:

219, #306, #504, #405, #407 -- agree to invalidate based on the reasoning in the escalation comment.
252 is already considered on its escalation.
413 -- As I understand, the front-running is still possible (until the launch of IOTA V2 with no mempool), but unreliable. But, the report doesn't have sufficient proof of the issue considering there are other constraints of the lock duration, for example. Hence, I agree it should be invalid.
419 -- agree with the escalation comment, I believe it's precise.
I agree that issues #6, #530, #362 and #253 are a different family. #6 is escalated, so the discussion about them will be continued under #6.

Planning to accept the escalation and apply the changes above.

WangSecurity commented 2 months ago

Result: High Has duplicates

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status:

chinmay-farkya: accepted

sherlock-admin2 commented 1 month ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-06-magicsea-judging

cocacola - Voting does not take into account end of staking lock period #166

Voting does not take into account end of staking lock period

Summary

Vulnerability Detail

Proof of Concept

Impact

Code Snippet

Tool used

Recommendation

306 is not a duplicate and is invalid as it ties to the same reasoning for #219 above

504 is again not a duplicate as it ties to the same reasoning for #219 above

405 is again not a duplicate as it ties to the same reasoning for #219 above

407 is not a duplicate and is invalid due to the same reason as #219, cited above.

413 is invalid because frontrunning is not possible on iota

306 is not a duplicate and is invalid as it ties to the same reasoning for #219 above

504 is again not a duplicate as it ties to the same reasoning for #219 above

405 is again not a duplicate as it ties to the same reasoning for #219 above

407 is not a duplicate and is invalid due to the same reason as #219, cited above.

413 is invalid because frontrunning is not possible on iota

219, #306, #504, #405, #407 , #419; This group is considered invalid under "Admin Input and responsibility", I agree.

252 low/Invalid , I agree (There is an escalation process in 252, so 252 should be evaluated as low/invalid.)

413 I do not agree.(The verdict is that front-running on IOTA is valid)

6, #530, #362, #253 Set includes . I do not agree

413 is invalid because frontrunning is not possible on iota

6, #530, #362, #253 Set includes . I do not agree

413 I do not agree.(The verdict is that front-running on IOTA is valid)

6 and #166

219, #306, #504, #405, #407 -- agree to invalidate based on the reasoning in the escalation comment.

413 -- As I understand, the front-running is still possible (until the launch of IOTA V2 with no mempool), but unreliable. But, the report doesn't have sufficient proof of the issue considering there are other constraints of the lock duration, for example. Hence, I agree it should be invalid.

419 -- agree with the escalation comment, I believe it's precise.