Incorrectly set _maxTime to be in line with the locking maxTime of each veToken could render the deposit of this contract to be unfunctional or even freeze assets inside the contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L38-L51

Vulnerability details

since _maxTime needs to be manually input in the constructor with no other ways of changing it, if the owner inputs the _maxTime that is higher than the capacity of each vaults of veTokens, it will cause IVoteEscrow(escrow).increase_unlock_time(_value); to get rejected.

In the mild case when a user initially locks the asset during depositing, the contract will simply revert the transaction.

In the severe case when a user does not lock the asset during depositing, the asset will end up locked in the contract since noone will be able to succesfully call lockVeAsset. This will cause the asset to be locked up with no way of withdrawing it. Even though a user will still get benefits from the minted reward, a contract will not be able to receive any benefits since it can't utilize the locked asset in VeAssetDepositor.

Proof of Concept

1.The owner initially sets _maxTime in constructor to be 126489600 (864003664) instead of 126144000 (864003654) which is the maximum time that a user can deposit in curve

2.A user deposit veAsset but deferring the locking to save gas

3.The veAsset is transferred from a user to the contract and the contract mint a reward token to the user

*Above shows that the deposit will get reverted if lockAmount is more than 4 years in curve's VotingEscrow

4.The veAsset will not be able to move to the staking contract because when VoterProxy calls increase_unlock_time in the targeted vault, the call will get reverted due to exceeded _value of unlockAt

mitigation

The owner should set the value of _maxTime in advance for each veAsset and not relying on manual inputting during the constructoring as the risk of misconfiging ot is high. Otherwise the contract should add an emergency measure that can help change _maxTime but this function needs to be protected with the highest security (eg. with timelock and multisig).

[solvetony]

We can set it by a function instead of a constructor. Middle risk.

[GalloDaSballo]

The warden has shown how, due to misconfiguration a lock may not be created, causing tokens to be stuck in the contract.

We can confirm that a revert would happen by checking VotingEscrow

Because this is contingent on a wrong configuration I believe Medium Severity to be more appropriate.