sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

Minato7namikazi - unhandled missed case in merge function #546

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

Minato7namikazi

High

unhandled missed case in merge function

Vulnerability Detail

function merge(uint _from, uint _to) external {
    require(attachments[_from] == 0 && !voted[_from], "attached");
    require(_from != _to);
    require(_isApprovedOrOwner(msg.sender, _from));
    require(_isApprovedOrOwner(msg.sender, _to));

    LockedBalance memory _locked0 = locked[_from];
    LockedBalance memory _locked1 = locked[_to];
    uint value0 = uint(int256(_locked0.amount));
    uint end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;

    locked[_from] = LockedBalance(0, 0);
    _checkpoint(_from, _locked0, LockedBalance(0, 0));
    _burn(_from);
    _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
}

The bug here is that the function doesn't properly handle the case where the _to token has a longer lock time than the _from token.

  1. It correctly calculates the new end time as the maximum of the two lock end times.
  2. However, when calling _deposit_for, it uses value0 (the amount from the _from token) and end (the new end time), but it doesn't adjust the amount based on the potentially extended lock time.

This could lead to an unfair advantage where a user could merge a token with a shorter lock time into one with a longer lock time, effectively increasing their voting power without extending the lock on the merged-in tokens.

Code Snippet

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

Tool used

Manual Review

nevillehuang commented 3 months ago

Invalid, this is describing the exact funcitonality of merge