PCV_CONTROLLER_ROLE can be granted by the deployer of Core and withdraw any tokens form PCVDeposit contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/PCVDeposit.sol#L17-L45

Vulnerability details

Impact

    function withdrawERC20(
        address token,
        address to,
        uint256 amount
    ) public virtual override onlyPCVController {
        _withdrawERC20(token, to, amount);
    }

    function _withdrawERC20(
        address token,
        address to,
        uint256 amount
    ) internal {
        IERC20(token).safeTransfer(to, amount);
        emit WithdrawERC20(msg.sender, token, to, amount);
    }

    /// @notice withdraw ETH from the contract
    /// @param to address to send ETH
    /// @param amountOut amount of ETH to send
    function withdrawETH(address payable to, uint256 amountOut)
        external
        virtual
        override
        onlyPCVController
    {
        Address.sendValue(to, amountOut);
        emit WithdrawETH(msg.sender, to, amountOut);
    }

Using the withdrawERC20() and withdrawETH() function of PCVDeposit, an address with PCV_CONTROLLER_ROLE can withdraw any tokens form PCVDeposit contract.

If the private key of the deployer or an address with the PCV_CONTROLLER_ROLE is compromised, the attacker will be able to withdraw any tokens.

We believe this is unnecessary and poses a serious centralization risk.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/PCVDeposit.sol#L17-L45

Tools Used

None

Recommended Mitigation Steps

In the withdrawERC20 function, check that the token is not cToken or cToken.underlying()

[ElliotFriedman]

This is expected behavior. The PCV Controller and Governor roles are privileged roles in the system and their compromise can result in loss of all funds.

[ElliotFriedman]

PCV Controllers will not be controlled by a single private key, instead they will be behind token voting governance timelocks

[jack-the-pug]

Dup #66