A user's tokens could be locked for an extended duration beyond their intention and without their control

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L155

Vulnerability details

Description

The LockingMultiRewards allows anyone to stake MagicLP tokens in return for rewards.
LP tokens are sent with either functions below:

function stake(uint256 amount, bool lock_) public whenNotPaused {
    _stakeFor(msg.sender, amount, lock_);
}
/// @notice Locks an existing unlocked balance.
function lock(uint256 amount) public whenNotPaused {
    if (amount == 0) {
        revert ErrZeroAmount();
    }
    _updateRewardsForUser(msg.sender);
    _balances[msg.sender].unlocked -= amount;
    unlockedSupply -= amount;
    _createLock(msg.sender, amount);
}

This will lead to _createLock(), which calculates the nextUnlockTime():

function _createLock(address user, uint256 amount) internal {
    Balances storage bal = _balances[user];
    uint256 _nextUnlockTime = nextUnlockTime();

function nextUnlockTime() public view returns (uint256) {
    return nextEpoch() + lockDuration;
}

Note that nextEpoch() would always return the next 1-week (or reward duration) slot.

function epoch() public view returns (uint256) {
    return (block.timestamp / rewardsDuration) * rewardsDuration;
}
function nextEpoch() public view returns (uint256) {
    return epoch() + rewardsDuration;
}

An issue arises because the user cannot specify the latest desired unlock time. This opens the path for the tokens to be locked for longer than expected, which may have significant impact for users if they need the funds. Consider the following case, where x is week number:

• It is day 7x + 6 (one day from nextEpoch)
• Assumed unlock time is 7x + 7 + lockDuration
• The TX does not execute in next 1 day for any reason (gas price went up, validator does not include TX, etc)
• It is now day 7x+7, another epoch starts. Now nextEpoch is 7x + 14
• Executed unlock time is 7x + 14 + lockDuration

This means user's funds are locked for an additional 7 days more than expected.

Note that delayed execution of a user's TX has always been considered in scope, certainly for Med severity impacts:
- https://solodit.xyz/issues/m-13-interactions-with-amms-do-not-use-deadlines-for-operations-code4rena-paraspace-paraspace-contest-git
- https://solodit.xyz/issues/m-04-lack-of-deadline-for-uniswap-amm-code4rena-asymmetry-finance-asymmetry-contest-git
- https://solodit.xyz/issues/m-03-missing-deadline-param-in-swapexactamountout-allowing-outdated-slippage-and-allow-pending-transaction-to-be-executed-unexpectedly-code4rena-pooltogether-pooltogether-git

Essentially the locking function lacks a deadline parameter similar to swapping functions, and the impact is temporary freeze of funds.

Impact

A user's tokens could be locked for an extended duration beyond their intention and without their control.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a latestUnlockTime deadline parameter for the locking functions.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as primary issue

[141345]

extend lock by others

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

0xCalibur (sponsor) acknowledged

[0xCalibur]

Abracadabra-money/abracadabra-money-contracts#138

[c4-sponsor]

0xCalibur marked the issue as disagree with severity

[0xCalibur]

Should be a low

[thereksfour]

Here the latest discussion on transaction delays.
code-423n4/2024-02-uniswap-foundation-findings#331 (comment)

And I would consider it QA which is due to low likelihood and medium impact (temporary freeze of funds)

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[trust1995]

Hi,

I would like to argue that likelihood is medium and impact is high.
likelihood - Assuming there will be many stakers on the platform, at any moment there could be a request close to the end of epoch. The closer the request is to the end of an epoch, the more likely it is a TX will not be executed until the next one.
Additionally, the users of the stake() function are completely different in understanding/sophistication from the ones in the Unistaker contest. They cannot be assumed to understand TX delays, requirement of invalidating one's TX if it is undesirable, etc.
Additionally one needs to take into consideration that gas prices are fluctuating, so censorship of a TX is absolutely not required for the FoF to occur. It just needs to be on a local low-point for the duration until the next interval for the impact to be achieved. Again, consider that this is not a targetted attack , the issue is a constant probabilistic leak that given enough time will materialize.

For impact, temporary FoF is a very serious issue, on the low end of the High impact (imo). Consider for example that a user assumes their tokens will be available at the unlock period in order to pay off a loan, and because of the FoF they will now default. This is just one example of hundred of different very serious scenarios that would occur to a user that assumes funds will be available in the future.

Considering all the arguments above, I believe the finding to be between M and H severity, rather than between L and M severity.

[thereksfour]

Agreed, the lock duration is jumping, which may compromise user

[c4-judge]

This previously downgraded issue has been upgraded by thereksfour

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report