AuraClaimZap's claimRewards can permanently freeze user Aura funds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraClaimZap.sol#L221-L227

Vulnerability details

If claimRewards is called with depositCvxMaxAmount > 0 and Options.LockCvx == false, the up to depositCvxMaxAmount AURA tokens are pulled from the user, but never get staked.

There looks to be no way to retrieve Aura tokens ended up on AuraClaimZap balance this way. I.e. there is a missing part of logic for the positive depositCvxMaxAmount and false LockCvx case.

The impact is the permanent fund freeze given inaccurate input from the user. As the probability of such an input isn't negligible (i.e. the requirement that flags should match or else funds are frozen leave big enough space for user mistake) and system logic is violated here as Zap is a fund moving utility contract not supposed to hold tokens and not having rescue functionality, so setting the severity to high.

Proof of Concept

Aura (CVX) will be lost for a user if depositCvxMaxAmount > 0 and uint256(Options.LockCvx) is false as claimRewards -> _claimExtras call will pull from the user, but never use the Aura funds:

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraClaimZap.sol#L221-L227

if (cvxBalance > 0) {
    //pull cvx
    IERC20(cvx).safeTransferFrom(msg.sender, address(this), cvxBalance);
    if (_checkOption(options, uint256(Options.LockCvx))) {
        IAuraLocker(locker).lock(msg.sender, cvxBalance);
    }
}

The fund freeze looks to be permanent and there is no mechanics in place to retrieve them as Zap is a pass-through utility contract that aren't supposed to hold funds, so there is no rescue function.

Particularly, the Aura funds to be removed from user's balance and will not be accounted in the future runs:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraClaimZap.sol#L219

uint256 cvxBalance = IERC20(cvx).balanceOf(msg.sender).sub(removeCvxBalance);

Notice that Convex.ClaimZap utilize (stake) the same CVX funds when Options.LockCvx is false:

https://github.com/convex-eth/platform/blob/main/contracts/contracts/ClaimZap.sol#L193-L202

Recommended Mitigation Steps

If there be auraRewards introduced then the same approach as used in Convex.ClaimZap can be enabled.

Meanwhile consider pulling the funds from the user only when all the required flags are set (i.e. depositCvxMaxAmount > 0 and Options.LockCvx is true):

if (cvxBalance > 0 && _checkOption(options, uint256(Options.LockCvx))) {
    //pull cvx
    IERC20(cvx).safeTransferFrom(msg.sender, address(this), cvxBalance);
    IAuraLocker(locker).lock(msg.sender, cvxBalance);
}

[0xahtle7]

Duplicated of https://github.com/code-423n4/2022-05-aura-findings/issues/108 Duplicated of https://github.com/code-423n4/2022-05-aura-findings/issues/263

[dmvt]

Duplicate of #108