In VoterProxy the address veAsset is not added to protectedTokens[] list so it's possible to call withdraw() with veAsset address by stash protocol and withdraw veAsset Balance of VoterProxy

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L108-L121

Vulnerability details

Impact

Stash contract can withdraw extra incentive reward tokens out of VoterProxy contract. and Stash calls withdraw function of VoterProxy for extra reward tokens of gauges. but veAsset was in gauges reward tokens then Stash will call withdraw() with veAsset address and it will withdraw veAsset balance of VoterProxy which can make other logics wrong and funds would be wrongly distributes. because veAsset is used for other things in VoterProxy

Proof of Concept

This is withdraw() function code in VoterProxy:

    //stash only function for pulling extra incentive reward tokens out
    function withdraw(IERC20 _asset) external returns (uint256 balance) {
        require(stashPool[msg.sender] == true, "!auth");

        //check protection
        if (protectedTokens[address(_asset)] == true) {
            return 0;
        }

        balance = _asset.balanceOf(address(this));
        _asset.safeTransfer(msg.sender, balance);
        return balance;
    }

As you can see it's only callable by Stash contract and if protectedTokens[address(_asset)] == true then code don't do anything. for any lpToken and gauge address contract sets protectedTokens[address(_asset)] = true so if they were in extraReward address, contract don't withdraw them because they are used in stacking and other logics. but contract don't set veAsset as protectedToken so it's possible that Stash call withdraw() with veAsset address and withdraw VoterProxy's veAsset balance which can cause fund lose because veAsset token has been used to stake in VoterEscrow and also other rewarding logics if Stash withdraw veAsset balance of VoterProxy those logics will be broken and funds would distributes wrongly.

Tools Used

VIM

Recommended Mitigation Steps

add veAsset token address to protectedTokens too.

[solvetony]

We have different BaseReward pool and VoterProxy for different projects, so this will not happen. within a project, the veAsset protected token will not be a extra reward token.

[GalloDaSballo]

The finding is missing an explanation as to how the veToken would be stuck in the contract (seems to always be locked away) and how the Stash would be calling to sweep such veToken away.

From experience I believe a veToken (e.g CRV) can be a reward from gauges, in that case the Stash will be withdrawing that token away from the VoterProxy

The rewards would then be claimable and sent to the Stash, which is the correct behaviour.

I can foresee a situation in which the contract haven't been unlocked for longer than a lock duration (meaning the locks are broken) and somebody else claimed on the behalf of the VoterProxy.

However, the contracts will trigger a relock on each deposit (see other findings), and the warden doesn't seem to mention a way for tokens to be lost via any of these.

For those reasons, I believe the finding to be invalid