All sort of staking like S1, S2 and LP stakes are not checking the actual time that is going to be locked.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L875-L1174

Vulnerability details

Impact

1. user can stake and withdraw. Effectively enjoying the points , bonus points, rewards without actually staking.
2. Contract assumes that the staking is locking the funds, but actually not.

Proof of Concept

Lets see one of the staking functions, _stakeS1Citizen This function would be called with valid _timelock. For input time , the time lock duration is calculated busing bit right shift. uint256 timelockDuration = _timelock >> 128; When we look at how this calculated it _timelock / (2 power 128). But for the given time, it is possible the above calculation would give output as zero. Further down, the codes do not check this lock duration and tries to update the locked duration as

    unchecked {
        citizenStatus.points =
            identityPoints * vaultMultiplier * timelockMultiplier /
            _DIVISOR / _DIVISOR;
        citizenStatus.timelockEndTime = block.timestamp + timelockDuration;

Though the time validity check is done in stake() at line 1221. But it is not guaranteed in above mentioned scenario when truncation happens.

Tools Used

Manual review.

Recommended Mitigation Steps

The fix could be simple , just validate the lock duration in all sort of staking. S1, S2, LP

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope

[hansfriese]

Administrative misconfiguration are out of scope according to readme

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-b

[hansfriese]

will keep for the sponsor's review

[TimTinkers]

Lets see one of the staking functions, _stakeS1Citizen This function would be called with valid _timelock.

How exactly would it be possible to call a private function like _stakeS1Citizen without first passing through the stake function's timelock validation?

// Validate that the selected timelock option is valid for the staked asset.
uint256 timelockOption = timelockOptions[_assetType][_timelockId];
if (timelockOption == 0) {
    revert InvalidTimelockOption(uint256(_assetType), _timelockId);
}

[c4-sponsor]

TimTinkers marked the issue as sponsor disputed

[c4-judge]

hansfriese marked the issue as grade-c

[aktech297]

@TimTinkers , I am also aware that _stakeS1Citizen can not be called directly since it is private. I have mentioned this in my report as Though the time validity check is done in stake() at line 1221. But it is not guaranteed in above mentioned scenario when truncation happens.. @hansfriese , @TimTinkers , I am trying to convey that the though there is validity check done in code to avoid zero stake time, but that would be not be sufficient. All type of stake functions are called through the stake() function with timelockOption This timelockOption is validated in lines . The timelockOption` is directly compared with zero.

But when we see one of the function, lets see the _stakeS1Citizen , at line, the lock duration is updated with right shift value uint256 timelockDuration = _timelock >> 128;

There is difference in how the time value is checked in one place and how it is used in another place. Staker can easily bypass this by providing the timelockOption > 0 in stake function, but when it right shifted, the actual duration could be zero. In right shift the calculation is, the _timelock is divided by 2 power 128.

So the correct way to ensure the valid time duration is by taking right shifted value in for timelockOption in stake function and checking it with zero.

[hansfriese]

@aktech297 In the stake() function, the stakers just set _assetType and _timelockId. And timelockOption is derived from timelockOptions that was set by admin. So the below non-zero check is to ensure the timelock option was set by admin properly and we don't need to verify further as the detailed value will be set by admin and the admin misconfiguration is out of scope.

[aktech297]

@hansfriese Yes you are correct, the timlockOptions values are configured in configureTimelockOptions . but when I see the sponsor's comment

How exactly would it be possible to call a private function like _stakeS1Citizen without first passing through the stake function's timelock validation?

// Validate that the selected timelock option is valid for the staked asset.
uint256 timelockOption = timelockOptions[_assetType][_timelockId];
if (timelockOption == 0) {
    revert InvalidTimelockOption(uint256(_assetType), _timelockId);
}

I believe, they are relying on the above check to ensure the valid duration. So, I am not sure how they are doing it. If front end is doing it with right shifted value, then it is fine. Otherwise it would be definitely an issue.

but when I hear the above comments from sponsors, I just wanted to double check and make sure that they are correctly validating in front end.

Thanks for your kind response.

[hansfriese]

@aktech297 I understand your point. I think the sponsor thought your submission was saying the stakers can call _stakeS1Citizen() directly to bypass the validation and left such a comment. In a word, this issue is about the admin configuration.