MaverickV2VotingEscrow Manual Review Findings

MVW-01M: Inexistent Validation of Timepoint

Type	Severity	Location
Input Sanitization		MaverickV2VotingEscrow.sol:L80

Description:

The MaverickV2VotingEscrow::createIncentiveBatch function will not sanitize the timepoint for which the incentive batch is created for, potentially resulting in loss of funds.

Impact:

A severity of minor has been assigned as the MaverickV2VotingEscrow::createIncentiveBatch function is publicly invoke-able, and can result in loss of funds under certain timepoint configurations.

Example:

Copy
57/// @inheritdoc IMaverickV2VotingEscrowBase
58function createIncentiveBatch(
59    uint128 amount,
60    uint48 timepoint,
61    uint128 stakeDuration,
62    IERC20 incentiveToken
63) public returns (uint256 index) {
64    if (amount == 0) revert VotingEscrowInvalidAmount(amount);
65    if (stakeDuration != 0) {
66        if (incentiveToken == baseToken) {
67            _checkDuration(stakeDuration);
68        } else {
69            // if not base token, stakeDuration should be zero
70            revert VotingEscrowInvalidDuration(stakeDuration, 0, 0);
71        }
72    }
73
74    index = incentiveBatchCount;
75
76    _tokenIncentiveTotals[incentiveToken].totalIncentives += amount;
77
78    _incentiveBatches[index].totalIncentives = amount;
79    _incentiveBatches[index].incentiveToken = incentiveToken;
80    _incentiveBatches[index].claimTimepoint = timepoint;
81    _incentiveBatches[index].stakeDuration = stakeDuration;
82    incentiveBatchCount++;
83
84    incentiveToken.safeTransferFrom(msg.sender, address(this), amount);
85    emit CreateNewIncentiveBatch(msg.sender, amount, timepoint, stakeDuration, incentiveToken);
86}

Recommendation:

We advise the code to ensure that the Votes::getPastTotalSupply of the timepoint is non-zero, preventing timepoints that have no stakes attached from being utilized.

This check will simultaneously prevent future timepoint values from being configured, however, if future timepoints are a desirable business trait the Votes::getPastTotalSupply could be invoked solely when the timepoint is in the past and additional considerations can be introduced once the timepoint elapses (i.e. refunding the funds).

Alleviation (07ad29f773f16bdfbae3d97d3a7c2f9d64866093):

The Maverick Protocol team stated that they deem the onus to be on the user to select an appropriate timepoint for the incentive they are creating, opting to instead acknowledge this exhibit.

MVW-02M: Incorrect Extension of Existing Lockup

Type	Severity	Location
Logical Fault		MaverickV2VotingEscrow.sol:L113

Description:

The MaverickV2VotingEscrow::claimFromIncentiveBatchAndExtend function will overwrite whatever lockup was previously present at the lockupId value rather than extending it, rendering the mechanism insecure and resulting in loss of funds.

The VotingEscrow.Incentives.t.sol file attempts to test this case via the VotingEscrowIncentiveTest::testIncentiveExtend function, however, the test is invalid as it checks that the lockup.votes are greater than the lockup0.votes instead of ensuring that the lockup.votes is equal to the lockup0.votes plus the claimed reward. To illustrate the failure in the test suite directly, we advise L269 of the VotingEscrowIncentives.t.sol to be replaced by the following statements:

Copy
IMaverickV2VotingEscrow.Lockup memory estimatedLockup = ve.previewVotes(claimAmount + ONE_128, 52 weeks);
assertEq(lockup.votes, estimatedLockup.votes);

Impact:

Any lockupId attempted to be extended via the MaverickV2VotingEscrow::claimFromIncentiveBatchAndExtend function will result in total loss of the underlying collateral backing the lockup due to it being overwritten rather than extended.

Example:

Copy
102/// @inheritdoc IMaverickV2VotingEscrowBase
103function claimFromIncentiveBatchAndExtend(
104    uint256 batchIndex,
105    uint256 lockupId
106) public returns (Lockup memory lockup, uint128 claimAmount) {
107    uint256 stakeDuration;
108    IERC20 incentiveToken;
109    (claimAmount, stakeDuration, incentiveToken) = _claim(batchIndex);
110
111    if (incentiveToken == baseToken && stakeDuration != 0) {
112        // no need to transfer; the base assets are already on this contract
113        lockup = _stake(claimAmount, stakeDuration, msg.sender, lockupId);
114    } else {
115        revert VotingEscrowInvalidExtendIncentiveToken(incentiveToken);
116    }
117}

Recommendation:

We advise the extension mechanism to replicate the statements within the VotingEscrow::_extend function sans the inward token transfer, ensuring the lockupId is properly extended with the newly claimed incentive funds.

Alleviation (07ad29f773f16bdfbae3d97d3a7c2f9d64866093):

The code was updated to utilize the VotingEscrow::_extend function which has been updated to no longer transfer tokens inward, alleviating this exhibit in full.