search

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

4 stars 0 forks source link

`InfinityStaker` - unlocked tokens can count as locked #287

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

After a user's lock is expired, he can withdraw but his tokens still count in in StakePower

Proof of Concept

getUserStakePower uses userstakedAmounts[user][Duration.XX].amount, and does not check if the tokens are vested or not.

So for example, if Alice stakes 1 token for 3 months, after 3 months she has 1 token she can instantly withdraw but getUserStakePower still returns 2.

First I am not sure this is intended, and is not in the interest of the protocol as you'd like users to increase their commitment and their lock duration.

Then, in an extreme case, this could lead to a wrapper contract where users would stake through the contract, and then sell the property of this contract instead of withdrawing. Like Alice would sell her unlocked 3 months vested token for a premium instead of withdrawing.

Recommended Mitigation Steps

Correct getUserStakePower to count expired staked amounts as unlocked ones.

nneverlander commented 2 years ago

Withdrawal will reduce the userstakedAmounts[user][Duration.XX].amount so getUserStaekPower will not be 2

Picodes commented 2 years ago

@nneverlander to clarify: this issue is precisely on the case where tokens are unlocked but the user does not unstake

HardlyDifficult commented 2 years ago

This seems like a design choice. The power could be intentionally be weighted this way because although the tokens could be withdrawn, the user did choose to lock them up for that period of time.

I closed a very similar report as invalid, however the wrapper is an interesting consideration. Lowering risk and merging with the warden's QA report #294