[WP-H23] `YVaultLPFarming.sol` Pending JPEG rewards can be stolen by attacker

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L154-L163

Vulnerability details

When the pending yield/rewards can be triggered and cause a surge of rewardPerShare for the stakers, there is a well-known attack vector is to take a large portion of the shares before the surge, then trigger the harvest and exit immediately after to steal part of the newly added rewards.

This can be done in about 1 block of time, and with a sufficient amount of funds, a large portion of the pending rewards can be stolen.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L154-L163

    /// @dev Updates this contract's rewards state
    function _update() internal {
        if (block.number <= lastRewardBlock) return;

        lastRewardBlock = block.number;

        if (totalStaked == 0) return;

        (accRewardPerShare, previousBalance) = _computeUpdate();
    }

Anyone can call vault.withdrawJPEG() to get jpeg rewards to YVaultLPFarming:

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L186-L190

    /// @notice Allows anyone to withdraw JPEG to `farm` 
    function withdrawJPEG() external {
        require(farm != address(0), "NO_FARM");
        controller.withdrawJPEG(address(token), farm);
    }

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/Controller.sol#L156-L166

    /// @notice Allows the vault for token `_token` to withdraw JPEG from
    /// `_token`'s strategy
    /// @param _token The strategy's token
    /// @param _to The address to send JPEG to
    function withdrawJPEG(
        IERC20 _token,
        address _to
    ) external {
        require(msg.sender == vaults[_token], "NOT_VAULT");
        strategies[_token].withdrawJPEG(_to);
    }

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L300-L307

    /// @notice Allows the controller to claim JPEG rewards from Convex
    /// and withdraw JPEG to the `_to` address
    /// @param _to The address to send JPEG to
    function withdrawJPEG(address _to) external onlyController {
        // claim from convex rewards pool
        convexConfig.baseRewardPool.getReward(address(this), true);
        jpeg.safeTransfer(_to, jpeg.balanceOf(address(this)));
    }

PoC

Given:

• Existing users staked 10M vault tokens in total;
• yVault got 1M jpeg of pending rewards.

The attacker can:

1. Borrow and deposit() 10M vault tokens;
2. Call vault.withdrawJPEG() to harvest the jpeg rewards to YVaultLPFarming;
3. Call withdraw() to get back 10M vault tokens and repay the loan;
4. Call claim() to claim 0.5M jpeg rewards;

Recommendation

Consider changing reward to the gradual release model:

function notifyRewardAmount(uint256 _reward, uint256 _duration) external onlyVault {
    rewardRate = _reward.div(_duration);
    lastUpdateTime = block.timestamp;
    periodFinish = block.timestamp.add(_duration);
    ...
}

[spaghettieth]

JPEG rewards not in the contract are already accounted for by calling vault.balanceOfJPEG, which also returns the total amount of JPEG rewards claimable by the underlying strategy from Convex. These rewards are already accounted for in accRewardPerShare before withdrawJPEG is called.

[dmvt]

Sponsor is correct. Invalid.