veToken Finance contest - Kumpa's results

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

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).

Lines of code

https://github.com/convex-eth/platform/blob/1f11027d429e454dacc4c959502687eaeffdb74a/contracts/contracts/Booster.sol#L145-L161

Vulnerability details

Booster.setFees does not specify ranges for each incentives even though there is a comment stating that these values should be in certain ranges. This loop hole enable malicious feeManager to set fee for his own benefit. If feeManager is the owner, he could make platformFee == MaxFee and reduce other incentives to zero. If feeManager is seperate from the owner and does not have an access to platform's treasury, he could set _callerFees == MaxFee and call earmarkRewards to collect the highest fee possible before setting it back to normal.

Proof of Concept

Mitigation

The owner should specify a clear range for each incentives and fees and make it a require function so that feeManager will not be able to arbitariry adjust it to his benefit (max and min can be adjusted )

*example the owner should add this ranges before setting up incentives and fees (max and min can be adjusted )

(require _lockFees >= 1000 && _lockFees <= 1500 && _stakerFees >= 300 && _stakerFees <= 600 && _callerFees >= 10 && _callerFees <= 100 && _platform <= 200)