VTVL contest - dipp's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L446-L451

Vulnerability details

Impact

Some tokens may be deployed through a proxy and so will have 2 entrypoints for its functions, the proxy contract and the implementation contract. Since any ERC20 token may be used in VTVLVesting.sol, these proxy tokens could be used.

The withdrawAdmin function in VTVLVesting.sol is used by an admin to withdraw the contract's vesting tokens (tokenAddress) that have not been allocated for vesting. An additional withdrawOtherToken function may also be used to withdraw any token from the VTVLVesting.sol contract that is not the vesting token. If a proxy token is used as the vesting token, an admin could call withdrawOtherToken with the tokenAddress's implementation contract as input to bypass the restrictions in withdrawAdmin, allowing the admin to withdraw all of the vesting tokens while the contract still has active and unfulfilled claims.

Although this could seem redundant, since an admin could just call revokeClaim on all active claims and then call withdrawAdmin to withdraw all vesting tokens, the withdrawOtherToken should not be able to withdraw any vesting tokens.

Proof of Concept

The withdrawOtherToken in VTVLVesting.sol:

    function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin {
        require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
        uint256 bal = _otherTokenAddress.balanceOf(address(this));
        require(bal > 0, "INSUFFICIENT_BALANCE");
        _otherTokenAddress.safeTransfer(_msgSender(), bal);
    }

The admin is able to bypass calling the proxy of the token by supply the address of its implementation contract as the _otherTokenAddress.

Recommended Mitigation Steps

A quick fix might be to check that vesting token's balance remains unchanged at the end of withdrawOtherToken. This would still allow admins to use any token as the vesting token, but would not allow withdrawOtherToken to transfer out vesting tokens.