oxchryston - Tokens sent to `manager` contracts will be lost `forever`.

[sherlock-admin]

oxchryston

medium

Tokens sent to manager contracts will be lost forever.

Summary

The function transferTokens is used to transfer tokens and recover tokens accidentally sent to the manager contracts but the wrong modifier was used in that function preventing it from being called by the Operator

Vulnerability Detail

The function transferTokensis guarded by the wrong modifier onlyExtension instead of the onlyOperator modifier. Even the comment above the function verifies this fact by stating it.

 /**
     * OPERATOR ONLY: Transfers _tokens held by the manager to _destination. Can be used to
     * recover anything sent here accidentally. In BaseManagerV2, extensions should
     * be the only contracts designated as `feeRecipient` in fee modules.
     *
     * @param _token           ERC20 token to send
     * @param _destination     Address receiving the tokens
     * @param _amount          Quantity of tokens to send
     */
    function transferTokens(address _token, address _destination, uint256 _amount) external onlyExtension {
        IERC20(_token).safeTransfer(_destination, _amount);
    }

Impact

Unless the extension contracts has a funtion to withdraw this assets it will be trapped forever in the manager contract leading to loss of funds.

Code Snippet

https://github.com/IndexCoop/index-coop-smart-contracts/blob/317dfb677e9738fc990cf69d198358065e8cb595/contracts/manager/BaseManagerV2.sol#L309-L320

Tool used

Manual Review

Recommendation

This can be simply corrected by changing the modifier on the function from onlyExtension to onlyOperator, this ensures that the funtion can be called by Operator.

function transferTokens(address _token, address _destination, uint256 _amount) external onlyOperator {
        IERC20(_token).safeTransfer(_destination, _amount);
    }