VaultHelper contract should never have tokens at the end of a transaction

[code423n4]

Handle

hrkrshnn

Vulnerability details

VaultHelper contract should never have tokens at the end of a transaction

The VaultHelper should not hold tokens at any point of time. Doing so makes the tokens available for anyone to withdraw.

Here's an example exploit allowing an attacker to withdraw any tokens that the VaultHelper contains. Assume that the VaultHelper holds a token named ExampleToken.

function withdrawVault(
                       address _vault,
                       address _toToken,
                       uint256 _amount
                       )
    external
{
    address _gauge = IVault(_vault).gauge();
    if (_gauge != address(0)) {
        IERC20(_gauge).safeTransferFrom(msg.sender, address(this), _amount);
        ILiquidityGaugeV2(_gauge).withdraw(_amount);
        IVault(_vault).withdraw(IERC20(_vault).balanceOf(address(this)), _toToken);
        IERC20(_toToken).safeTransfer(msg.sender, IERC20(_toToken).balanceOf(address(this)));
    } else {
        IERC20(_vault).safeTransferFrom(msg.sender, address(this), _amount);
        IVault(_vault).withdraw(_amount, _toToken);
        IERC20(_toToken).safeTransfer(msg.sender, IERC20(_toToken).balanceOf(address(this)));
    }
}

Step 1

Deploy contracts with the following interfaces:

interface FakeVault {
    //// Returns a valueless FakeGuage token
    function gauge() external returns(address);
}
interface FakeGuage {
    /// Does nothing
    function transferFrom(address, address, uint) external returns(bool);
    /// Does nothing
    function withdraw(uint) external;
}

Step 2

Attacker calls withdrawVault(address(FakeVault), address(ExampleToken), 0).

This transfers all of _toToken to the attacker.

The project seems to explicitly specify when a contract is not supposed to hold tokens. For example, in MetaVaultNonConverter.sol or Manager.sol. In the absence of such a remark, it is assumed that the contract is safe to hold tokens, which is incorrect, in this case.

Recommended Mitigation Steps

Document that the VaultToken is not supposed to hold tokens.

[Haz077]

I don't see a way that results in tokens being held in VaultHelper but I agree it should be documented as in Manager.sol and MetaVaultNonConverter.sol I believe this issue should be labeled as non-critical and documentation.

[GalloDaSballo]

Agree with Non-Critical Severity, this is a documentation issue and is a contract feature, not flaw