mstpr-brainbot - Ended locks can be extended

[sherlock-admin]

mstpr-brainbot

high

Ended locks can be extended

Summary

When a lock period ends, it can be extended. If the new extension 'end' is earlier than the current block.timestamp, the user will have a lock that can be unstaked at any time."

Vulnerability Detail

When the lock period ends, the owner of the expired lock can extend it to set a new lock end that is earlier than the current block.timestamp. By doing so, the lock owner can create a lock that is unstakeable at any time.

This is doable because there are no checks in the extendLock function that checks whether the lock is already ended or not.

PoC:

function test_ExtendLock_AlreadyEnded() external {
        uint256 amount = 100e18;
        uint256 duration = 5 days;

        _stake(amount, duration, alice, alice);

        // 5 days later, lock is ended for Alice
        skip(5 days + 1);

        (,, uint128 _ends,,) = veTRUF.lockups(alice, 0);

        // Alice's lock is indeed ended
        assertTrue(_ends < block.timestamp, "lock is ended");

        // 36 days passed  
        skip(36 days);

        // Alice extends her already finished lock 30 more days
        vm.prank(alice);
        veTRUF.extendLock(0, 30 days);

        (,,_ends,,) = veTRUF.lockups(alice, 0);

        // Alice's lock can be easily unlocked right away
        assertTrue(_ends < block.timestamp, "lock is ended");

        // Alice unstakes her lock, basically alice can unstake her lock anytime she likes
        vm.prank(alice);
        veTRUF.unstake(0);
    }

Impact

The owner of the lock will achieve points that he can unlock anytime. This is clearly a gaming of the system and shouldn't be acceptable behaviour. A locker having a "lock" that can be unstaked anytime will be unfair for the other lockers. Considering this, I'll label this as high.

Code Snippet

https://github.com/sherlock-audit/2023-12-truflation/blob/37ddbb69e0c7fb6510f1ec99162fd9172ec44733/truflation-contracts/src/token/VotingEscrowTruf.sol#L333-L366

Tool used

Manual Review

Recommendation

Do not let extension of locks that are already ended.

[ryuheimat]

https://github.com/truflation/truflation-contracts/pull/7 Disable to extend lock if already expired.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Shaheen commented:

Valid issue but Medium. Good Finding

[securitygrid]

Escalate I didn't sumbit this issue because I think this is not an issue: no funds loss, no core function broken. The reason is: Alice is still in the game and has not called unstake to exit the game.

[nevillehuang]

@ryuheimat I think @securitygrid has a point, and this seems similar to this issue here, might have slipped my judgement

https://github.com/sherlock-audit/2023-12-truflation-judging/issues/169

However both this and #169 indeed does break the "locking" mechanism, given it is expected that funds should be locked within escrows instead of allowing users to simply unlock immediately. I believe both should be at least medium/high severity issues, and could possibly be considered to be duplicates

[sherlock-admin2]

Escalate I didn't sumbit this issue because I think this is not an issue: no funds loss, no core function broken. The reason is: Alice is still in the game and has not called unstake to exit the game.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[detectiveking123]

This is definitely a valid issue, but it is unclear to me whether it should be labeled as medium or high.

[Czar102]

I believe this is a valid medium. Core functionality – rewarding points for locking tokens – is broken.

From my understanding, someone must have locked the tokens in the past and can receive points retroactively, meaning that they effectively can achieve flexible duration locks. This shouldn't be possible and is an important property of the system.

[Czar102]

Also, I don't think #169 is a duplicate. Can you explain in more detail why would that be the case? @nevillehuang

[nevillehuang]

@Czar102 because they have the same impact, that is there is no “lock” required. I.e. User can gain rewards and retain voting power while still being able to unstake at any time.

[Czar102]

I see now. So effectively, the extension of the lock mentioned here doesn't matter since the identity locking receives points anyway?

[nevillehuang]

@Czar102 yes as long as the lock expires, the user will have no incentive to continue locking, and even if they do, they can unlock the lock at anytime as mentioned here. So I believe they should be duplicated as valid Medium severity.

[Czar102]

Planning to duplicate these and make the severity Medium.

[aslanbekaibimov]

@Czar102

I see now. So effectively, the extension of the lock mentioned here doesn't matter since the identity locking receives points anyway?

It does matter. #82 describes how the user is able to have voting power greater than their original one, while always being able to withdraw. The fix is straightforward - to add a check for the lock's expiration.

On the other hand, #169 only allows users to keep their original voting power from the original lock. The fix of #82 also does not fix #169.

[0xunforgiven]

I don't believe they are duplicates. the only common thing between #82 and #169 is a precondition for the issues. they both happens when a lock expires. but impact and fix and bug place is totally different for them.

82 explains that after lock expires users can call extend lock and extend their lock. so the issue is in extend function and it can be fixed by simple check in extend function, but this fix won't fix the #169.

169 explains that after the lock expires users still have voting power and receive rewards. so the issue is in voting mechanism and reward distribution. this issue can't be fixed easily with current design. fixing this will not fix the #82.

multiple wardens reported these two as separate issues while some wardens missed one of them. issues can't be concluded from each other.

[mstpr]

Fix LGTM

[ryuheimat]

mstpr

Merging...

[Czar102]

Removing locks after they expire will both remove the voting power (locks don't exist anymore) and make it impossible to retroactively extend the lock (since any ended lock won't exist anymore). This is why I think these are duplicates.

[0xunforgiven]

@Czar102 regarding:

Removing locks after they expire

in current code only the stacker can remove their own lock. so why should they remove their expired lock? this was their attack to not remove the expired lock and receive the rewards and voting power. he wouldn't remove his own lock(he is the one performing the attack) and as result receive rewards for his expired lock.

the sponsor in the https://github.com/sherlock-audit/2023-12-truflation-judging/issues/82#issuecomment-1882784243 says that their fix for extending expire lock is this:

Disable to extend lock if already expired.

so issue #169 still exists in the code.

[Czar102]

I agree it still exists. I still think these issues are duplicates, though instead of fixing the root cause #169, the sponsor decided to mitigate the implication of #169 described here, in #82.

Please note that the sponsor acknowledged an unfixed duplicate of this issue #169, which has a different impact.

[Czar102]

Result: Medium Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• securitygrid: accepted