code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L433-L450

Vulnerability details

Impact

There is no check on the result of the delegate call to strategy, executing strategy.harvest(): address(strategy).delegatecall(abi.encodeWithSignature("harvest()"));

It means that if strategy.harvest() reverts for any reason, adapter.harvest() will silently fail.

The harvest function is used in:

_deposit(address caller, address receiver, uint256 assets, uint256 shares) _deposit function in AdapterBase.sol GitHub Link
_withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares) _withdraw function in AdapterBase.sol GitHub Link

These two internal functions are used by these four external functions:

deposit(uint256 assets, address receiver): deposit function in AdapterBase.sol GitHub Link
mint(uint256 shares, address receiver): mint function in AdapterBase.sol GitHub Link
withdraw(uint256 assets, address receiver, address owner): withdraw function in AdapterBase.sol GitHub Link
redeem(uint256 shares, address receiver, address owner): redeem function in AdapterBase.sol GitHub Link

Moreover, function harvest() public takeFees is public, with no access-control, which means that anyone can call it any time.

Depending on what the attached strategy is supposed to do, the functions executing adapter.harvest() could have unexpected results, can't say much as it depends on a strategy that hasn't been implemented yet.

Proof of Concept

I used the BeefyAdapter for this PoC, but it would work the same way with any adapter that inherits AdapterBase.

testFail_harvest_revert source code

The strategy used is the MockStrategyRevert.sol, its harvest function reverts with "StrategyReverted()" when called.
The PoC rely on on calling harvest() in such conditions that strategy.harvest() will be called. We expect that the transaction reverts but it actually won't as the logs show:

testFail_harvest_revert logs

Tools Used

Manual Review

Recommended Mitigation Steps

Check the result of the delegated call to strategy and revert in case of failure.

/**
* @notice Execute Strategy and take fees. Updates lastHarvest
* @dev Delegatecall to strategy's harvest() function. All necessary data is passed via `strategyConfig`.
* @dev Delegatecall is used in case any logic requires the adapters address as a msg.sender. (e.g. Synthetix staking)
*/

error StrategyFailed();

function harvest() public takeFees {
    if (
        address(strategy) != address(0) &&
        ((lastHarvest + harvestCooldown) < block.timestamp)
    ) {
        //lastHarvest update should also be done

        // solhint-disable
        //Delegated call no return value should be addressed
        (bool success, ) = address(strategy).delegatecall(
            abi.encodeWithSignature("harvest()")
        );
        if(!success) revert StrategyFailed();
    }

    emit Harvested();
}

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)