code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L72 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L262 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L268

Vulnerability details

Impact

Detailed description of the impact of this finding.

I noticed that the stake amount change in stake() is +=, which means that the project itself defaults to adding positions at the same duration, but the timestamp of the duration is overwritten by = directly!

e.g. Alice selected THREE_MONTHS for staking, and after some time Alice want to continue to add positions to THREE_MONTHS pledge, but the time is overwritten to the time of this position addition, which is extremely unreasonable, the correct timestamp calculation should start from position creation, so I think we should add a conditional judgment of position addition time here. For example, if the position is opened within a week, the timestamp of the original opening time will remain unchanged!

function stake(uint256 amount, Duration duration) external override nonReentrant whenNotPaused {
    require(amount != 0, 'stake amount cant be 0');
    require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
    // update storage
    userstakedAmounts[msg.sender][duration].amount += amount;
    userstakedAmounts[msg.sender][duration].timestamp = block.timestamp;
    // perform transfer
    IERC20(INFINITY_TOKEN).safeTransferFrom(msg.sender, address(this), amount);
    // emit event
    emit Staked(msg.sender, amount, duration);
  }

When unstake() is called to cancel a pledge, the timestamp is overwritten causing an abnormal delay in Alice's pledge unlock time. This is extremely unfair to the user, so I consider this a High-level issue.

function getVestedAmount(address user, Duration duration) public view returns (uint256) {
    uint256 amount = userstakedAmounts[user][duration].amount;
    uint256 timestamp = userstakedAmounts[user][duration].timestamp;
    // short circuit if no vesting for this duration
    if (timestamp == 0) {
      return 0;
    }
    uint256 durationInSeconds = _getDurationInSeconds(duration);
    uint256 secondsSinceStake = block.timestamp - timestamp;

    return secondsSinceStake >= durationInSeconds ? amount : 0;
  }

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Changed to the timestamp will not change if the position is added within a period of time (weeks).

nneverlander commented 2 years ago

This was a design decision. So assessment is low.

HardlyDifficult commented 2 years ago

Working as designed. The recommendation includes an arbitrary timeframe which would seem to be more confusing for users. Closing as invalid.

code-423n4 / 2022-06-infinity-findings