Popcorn contest - minhtrng's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L305-L314

Vulnerability details

Impact

In MultiRewardStaking.changeRewardSpeed the new rewardsEndTimetamp is calculated based on the current balance of reward tokens in the contract. However, a fraction of this balance might already be accrued and accounted as reward, but just has not been claimed yet. This can cause more tokens to be claimable than the contract contains, leading to a "first come first serve" scenario for users concerning rewards.

Proof of Concept

The function MultiRewardStaking.changeRewardSpeed uses the current reward token balance of the contract as input for the calculation of the new rewardsEndTimeStamp:

uint256 remainder = rewardToken.balanceOf(address(this));

uint32 prevEndTime = rewards.rewardsEndTimestamp;
uint32 rewardsEndTimestamp = _calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
);
rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;

The idea is that the current balance of reward tokens is what still needs to be distributed (as the name remainder implies). However, as mentioned under #Impact a fraction of this balance might already be attributed to users and is just unclaimed. Using the full balance will lead to rewardsEndTimeStamp being later than it should be, causing more tokens to be accounted for claiming than the contract holds:

function _calcRewardsEnd(
    ...
    //amount = rewardToken.balanceOf(address(this));
    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Only the fraction of reward tokens that has not yet accrued should be used to calculate the rewardsEndTimestamp when changing the rewardspeed. A separate variable might be necessary to track the amount of tokens that are eligible to be claimed. That variable would need to be incremented on all _accrueStatic calls that occur when totalSupply > 0.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L496-L499

Vulnerability details

Impact

The Vault contract syncs the highWaterMark on deposit, mint and withdraw, which can be used to avoid paying the performance fees, by performing any of these actions with a tiny amount. After Clarification with the sponsor, this was caused by a merge gone wrong and is not supposed to be there

Proof of Concept

The highWaterMark is used as a threshold to trigger the payment of a performance fee:

function accruedPerformanceFee() public view returns (uint256) {
    uint256 highWaterMark_ = highWaterMark;
    uint256 shareValue = convertToAssets(1e18);
    uint256 performanceFee = fees.performance;

    return
        performanceFee > 0 && shareValue > highWaterMark
            ? performanceFee.mulDiv(
                (shareValue - highWaterMark) * totalSupply(),
                1e36,
                Math.Rounding.Down
            )
            : 0;
}

The Vault contains the following modifier used for deposits, mints and withdraws:

modifier syncFeeCheckpoint() {
    _;
    highWaterMark = convertToAssets(1e18);
}

This modifier can be abused by performing tiny deposits, mints and withdraws to move the highWaterMark to current share values to avoid the triggering of the performance fee.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the syncFeeCheckpoint modifier and all its usages from Vault

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L308-L312 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L351-L360

Vulnerability details

Impact

The function _calcRewardsEnd is called with the previousEndTime as first parameter in MultiRewardStaking.changeRewardSpeed, which leads to wrong calculation of the new rewardsEndTimestamp, causing it to be later than it should be. This will lead to more rewards being accrued "on paper" than the contract contains.

Proof of Concept

The function MultiRewardStaking.changeRewardSpeed calls the _calcRewardsEnd function like this:

uint32 rewardsEndTimestamp = _calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
);

If prevEndTime > block.timestamp == true then the following block in calcRewardsEnd will be executed:

if (rewardsEndTimestamp > block.timestamp)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

A quick calculation why the lines above should not be executed in this case:

function _calcRewardsEnd(
uint32 rewardsEndTimestamp, // @note previousRewardsEndtime = 1000; block.Timestamp=700
uint160 rewardsPerSecond, //@note newRewardsPersecond = 10 (previous = 1) 
uint256 amount //@note currentRewardTokenBalance = 300
) internal returns (uint32) {
if (rewardsEndTimestamp > block.timestamp)
    //@note with values above: amount = 300 + 10*300 = 3300
    // this logic is only compatible with `fundReward` (where `amount` is meant to be added rewards, 
    //while the righthandside calculates current reward amounts left)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

//@note 700 + 3300/10 = 1030 (should be 730, since no new tokens have entered the contract)
    //will try to distribute 330*10 = 3300 tokens even though there are only 300 in the contract
    //enables "theft" of rewards or permanent (?) lock of reward tokens (for some scenarios where each participant is entitled to more than the actual balance)
return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Call calcRewardsEnd the same way as is done in the addRewardToken function: _calcRewardsEnd(0, rewardsPerSecond, amount); (first parameter = 0)