Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L95 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L257 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L303

Vulnerability details

Impact

Whenever STRATEGISTs want to switch from currently used strategy to another one, they are required to call the Controller.setStrategy function. This function is responsible for first withdrawing CRV and JPEG tokens from strategy contract into vault, then proceed to reset the mapping of CRV token to strategy.

However, while the correct method to withdraw JPEG token should be through strategy.withdrawJPEG, Controller attempts to perform withdrawal through strategy.withdraw(address(jpeg)).

The strategy.withdraw(address) function is originally intended to allow strategists to withdraw unrelated tokens that are accidentally deposited to strategies, and explicitly block withdrawal of JPEG tokens. This causes Controller.setStrategy to revert whenever it tries to change strategy and withdraw JPEG tokens from old instance.

Proof of Concept

Controller.setStrategy attempts to withdraw JPEG tokens in strategy through _current.withdraw(address(jpeg)) whenever a switch is requested

    function setStrategy(IERC20 _token, IStrategy _strategy) external onlyRole(STRATEGIST_ROLE){
        ...
        IStrategy _current = strategies[_token];
        if (address(_current) != address(0)) {
            ...
            _current.withdraw(address(jpeg));
        }
        ...
    }

However, the strategy.withdraw(address) function is not intended to be used for JPEG withdrawal and explicitly reverts if it finds target token to be JPEG

    function withdraw(IERC20 _asset) external onlyController returns (uint256 balance) {
        ...
        require(jpeg != _asset, "jpeg");
        ...
    }

This causes Controller.setStrategy to always revert in the case which an STRATEGISTs want to swap out an old strategy implementation

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use the correct withdrawJPEG function to withdraw JPEG

    function setStrategy(IERC20 _token, IStrategy _strategy)
        external
        onlyRole(STRATEGIST_ROLE)
    {
        ...
        IStrategy _current = strategies[_token];
        if (address(_current) != address(0)) {
            ...
            _current.withdrawJPEG(vaults[_token]);
        }
        ...
    }

code-423n4 / 2022-04-jpegd-findings