Similar functions from VotingEscrow have different permissions

Summary

The two functions deposit_for and increase_amount contain similar implementations. However, one of them requires the caller to be either the owner or an approved spender, while the other doesn't.

Vulnerability Detail

Besides the initial assertion of the increase_amount function, the two functions do the exact same thing. Initially, they both verify for a greater than zero value and for an existing, non-expired lock. The only difference is that the increase_amount implementation uses assert instead of require for the value check. They end up by calling _deposit_for with an almost identical set of parameters, the only difference being the final variable, the deposit type. However, by looking at the deposit_for implementation, the only difference is in the emitted event, that is not used by the implementation.

Therefore, there's no reason to have the approved spender / owner check in the increased_amount function, as an user can bypass it by calling the deposit_for function.

Impact

Low - inconsisten API.

Code Snippet

The implementation of the two functions, for ease of review:

function deposit_for(uint _tokenId, uint _value) external nonreentrant {
    LockedBalance memory _locked = locked[_tokenId];

    require(_value > 0); // dev: need non-zero value
    require(_locked.amount > 0, 'No existing lock found');
    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
    _deposit_for(_tokenId, _value, 0, _locked, DepositType.DEPOSIT_FOR_TYPE);
}

function increase_amount(uint _tokenId, uint _value) external nonreentrant {
    assert(_isApprovedOrOwner(msg.sender, _tokenId));

    LockedBalance memory _locked = locked[_tokenId];

    assert(_value > 0); // dev: need non-zero value
    require(_locked.amount > 0, 'No existing lock found');
    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

    _deposit_for(_tokenId, _value, 0, _locked, DepositType.INCREASE_LOCK_AMOUNT);
}

Tool used

Manual Review

Recommendation

Either remove the require for increase_amount, or add it for the deposit_for function.

sherlock-audit / 2024-06-velocimeter-judging

Funny Merlot Yeti - Similar functions from VotingEscrow have different permissions #710