search

sherlock-audit / 2024-06-velocimeter-judging

0 stars 0 forks source link

Audinarey - Locks can be created for less than 1 epoch (1 WEEK) #576

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

Audinarey

High

Locks can be created for less than 1 epoch (1 WEEK)

Summary

The ve LpToken has a minimum lock period of 1 epoch (1 weeks). However, a flaw allows malicious actors to lock their Ll tokens for less than 1 week, resulting in unfair FLOW emission reward distribution. This means a malicious actor can unfairly claim rewards without meeting the minimum 1-week lock requirement

Vulnerability Detail

The problem is that the unlock_time calculation in the internal _create_lock(...) function is rounded down and is the validation of the calculated unlock_time only checks if this time is more than the current time. This wrong

    function _create_lock(uint _value, uint _lock_duration, address _to) internal returns (uint) {
        uint unlock_time = (block.timestamp + _lock_duration) / WEEK * WEEK; // Locktime is rounded down to weeks

        require(_value > 0); // dev: need non-zero value
>>>     require(unlock_time > block.timestamp, 'Can only lock until time in the future');
        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 52 weeks max');

This can also be detrimental for honest lockers as because the calculated unlock time can be less than users intended lock time making the user lock for less than their intended period.

CODED POC Add the test case below to VotingEscrow.t.sol and run forge test --mt testCreateLockBelowOneWeek -vvv

    function testCreateLockBelowOneWeek() public {
        flowDaiPair.approve(address(escrow), TOKEN_1);
        vm.warp(((block.timestamp / ONE_WEEK ) *  ONE_WEEK) + 4 days + 3);
        uint256 lockDuration = 3 * 24 * 3600; // 3 days

        // Balance should be zero before and 1 after creating the lock
        assertEq(escrow.balanceOf(address(owner)), 0);
        escrow.create_lock(TOKEN_1, lockDuration); // create lock for 3 days
        assertEq(escrow.currentTokenId(), 1);
        assertEq(escrow.ownerOf(1), address(owner));
        assertEq(escrow.balanceOf(address(owner)), 1);
    }

Impact

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L839-L845

Tool used

Foundry test

Recommendation

In the _create_lock(...) function to ensure the check handles the full range of the users lock time parammeter and it must not be less than one week

    function _create_lock(uint _value, uint _lock_duration, address _to) internal returns (uint) {
-       uint unlock_time = (block.timestamp + _lock_duration) / WEEK * WEEK; // Locktime is rounded down to weeks

        require(_value > 0); // dev: need non-zero value
-        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
+        require(unlock_time == block.timestamp + _lock_duration, 'Can only lock until time in the future');
        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 52 weeks max');
nevillehuang commented 1 month ago

Invalid/low severity, there is no indication that the minimum lock period is 1 week, only a max of 52 weeks. Also, the rewards are scaled timewise and checkpointed accordingly based on end period, so there is no unfairness here when rewards are distributed within reward distributor contract

Audinarey commented 1 month ago

@nevillehuang

I agree with you, but this report is in two folds for visibility I'll paste the second part of the report below

This can also be detrimental for honest lockers as because the calculated unlock time can be less than users intended lock time making the user lock for less than their intended period.

A coded POC to back up this claim is pasted below:

    function testLockEndIsBroken() public {
        flowDaiPair.approve(address(escrow), TOKEN_1);
        vm.warp(((block.timestamp / ONE_WEEK ) *  ONE_WEEK) + 4 days);
        uint256 lockDuration = 7 * 24 * 3600; // lock for 7 days

        // Balance should be zero before and 1 after creating the lock
        assertEq(escrow.balanceOf(address(owner)), 0);
        uint lockedToken = escrow.create_lock(TOKEN_1, lockDuration); // create lock for 3 days
        (int128 amt, uint end) = escrow.locked(lockedToken);
        assertEq(end, block.timestamp + 3 days); // lock ends in the next 3 days

    }

Kindly review.

nevillehuang commented 1 month ago

@Audinarey Unfortunately, I still don't see how the PoC shows unfairness and address my comments here

Audinarey commented 1 month ago

@nevillehuang

Please don't get me wrong the unfairness is one fold which I agree with your comment, the second part of the report talks about broken core protocol functionality, in fact I mentioned in the impact that

honest users could be forced to lock for less than 1 week against their will due to rounding error

the POC shows that the create_lock(...) functionality is broken for honest users

I'ld appreciate @dawiddrzala input on this as well.

Audinarey commented 1 month ago

@nevillehuang

Please find below the POC for the second part of the report. Ad it to the VotingEscrow.t.sol and run forge test --mt testLockEndIsBroken -vvv

    function testLockEndIsBroken() public {
        flowDaiPair.approve(address(escrow), TOKEN_1);
        vm.warp(((block.timestamp / ONE_WEEK ) *  ONE_WEEK) + 4 days);
        uint256 lockDuration = 7 * 24 * 3600; // lock for 7 days

        // Balance should be zero before and 1 after creating the lock
        assertEq(escrow.balanceOf(address(owner)), 0);
        uint lockedToken = escrow.create_lock(TOKEN_1, lockDuration); // create lock for 3 days
        (int128 amt, uint end) = escrow.locked(lockedToken);
        assertEq(end, block.timestamp + 3 days); // lock ends in the next 3 days

    }