User can accidentally permanently freeze the staked funds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L79-L109

Vulnerability details

User facing changeDuration() function allows for setting any newDuration of a stake. However, only THREE_MONTHS, SIX_MONTHS and TWELVE_MONTHS durations are visible to the system in all the subsequent logic. If a user accidentally sets any other duration, the changeDuration() will run successfully, but the corresponding stake will be frozen within the contract.

Setting severity to be medium as that's a fund freeze case conditional on a user irregular input, which probability isn't low as typical user aren't aware that InfinityStaker ignores all durations besides 3 preset values.

Proof of Concept

changeDuration() allows user to set any newDuration of a stake:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L79-L109

  /**
   * @notice Change duration of staked tokens
   * @dev Duration can be changed from low to high but not from high to low. State updates are performed
   * @param amount Amount of tokens to change duration
   * @param oldDuration Old duration of the stake
   * @param newDuration New duration of the stake
   */
  function changeDuration(
    uint256 amount,
    Duration oldDuration,
    Duration newDuration
  ) external override nonReentrant whenNotPaused {
    require(amount != 0, 'amount cant be 0');
    require(
      userstakedAmounts[msg.sender][oldDuration].amount >= amount,
      'insufficient staked amount to change duration'
    );
    require(newDuration > oldDuration, 'new duration must be greater than old duration');

    // update storage
    userstakedAmounts[msg.sender][oldDuration].amount -= amount;
    userstakedAmounts[msg.sender][newDuration].amount += amount;
    // update timestamp for new duration
    userstakedAmounts[msg.sender][newDuration].timestamp = block.timestamp;
    // only update old duration timestamp if old duration amount is 0
    if (userstakedAmounts[msg.sender][oldDuration].amount == 0) {
      userstakedAmounts[msg.sender][oldDuration].timestamp = 0;
    }
    // emit event
    emit DurationChanged(msg.sender, amount, oldDuration, newDuration);
  }

Recommended Mitigation Steps

Consider adding the check to changeDuration() that newDuration belongs to the set of (THREE_MONTHS, SIX_MONTHS, TWELVE_MONTHS) and revert otherwise.

[HardlyDifficult]

Since enums do not auto revert when the input is out of range, the stake may be left in an unsupported state. Good catch! Agree with Medium risk here.

[KenzoAgada]

I've tested this and it seems that enums do revert on invalid input, am I missing something? You can run the following contract and input 3 to the function

pragma solidity 0.8.14;

contract TestEnum {
    enum ABC {
        A, B, C
    }

    function getEnum(ABC _a) public {
    }
}

[HardlyDifficult]

You're right @KenzoAgada! My mistake... guess it's time I should read over the Solidity docs again. Thanks for pointing this out (& incl a simple test).

Closing as invalid.

[dmitriia]

My bad, implied uint behavior