Direct claim of convex rewards causes rewards to get stuck

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L272-L284

Vulnerability details

Impact

ConvexTriCryptoStrategy does not take into account that rewards from Convex can be claimed directly on behalf of any address. All rewards that get into the strategy contract this way will get stuck and compounding of yield will be denied.

Proof of Concept

The ConvexTriCryptoStrategy allows the claim and conversion of Convex rewards in its compound function:

    function compound(bytes memory data) public {
        if (data.length == 0) return;
        (uint256[] memory rewards, address[] memory tokens) = _executeClaim(
            data
        );

        //iterate through rewards and swap to wETH ...

The _executeClaim function performs the claim and then checks the balance difference of the contract before and after to calculate the rewards:

//parse input and check balances before
...

zap.claimRewards(
    tempData.rewardContracts,
    tempData.extraRewardContracts,
    tempData.tokenRewardContracts,
    tempData.tokenRewardTokens,
    extrasTempData.depositCrvMaxAmount,
    extrasTempData.minAmountOut,
    extrasTempData.depositCvxMaxAmount,
    extrasTempData.spendCvxAmount,
    extrasTempData.options
);

uint256[] memory balancesAfter = new uint256[](tempData.tokens.length);
for (uint256 i = 0; i < tempData.tokens.length; i++) {
    balancesAfter[i] = IERC20(tempData.tokens[i]).balanceOf(
        address(this)
    );
}

uint256[] memory finalBalances = new uint256[](tempData.tokens.length);
for (uint256 i = 0; i < tempData.tokens.length; i++) {
    finalBalances[i] = balancesAfter[i] - balancesBefore[i];
}

return (finalBalances, tempData.tokens);

The ClaimZap contract (Link) called here is from Convex and allows a batch claim of rewards from rewardContracts, extraRewardContracts and tokenRewardContracts:

for(uint256 i = 0; i < rewardContracts.length; i++){
    IBasicRewards(rewardContracts[i]).getReward(msg.sender,true);
}
//claim from extra rewards
for(uint256 i = 0; i < extraRewardContracts.length; i++){
    IBasicRewards(extraRewardContracts[i]).getReward(msg.sender);
}

Looking at the getReward functions (here and here), it can be seen that anyone is allowed to claim for any address:

//BaseRewardPool (rewardContract)
function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
    uint256 reward = earned(_account);
    if (reward > 0) {
        rewards[_account] = 0;
        rewardToken.safeTransfer(_account, reward);
        IDeposit(operator).rewardClaimed(pid, _account, reward);
        emit RewardPaid(_account, reward);
    }

    //also get rewards from linked rewards
    if(_claimExtras){
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).getReward(_account);
        }
    }
    return true;
}

//VirtualRewardPool (extraRewards)
function getReward(address _account) public updateReward(_account){
    uint256 reward = earned(_account);
    if (reward > 0) {
        rewards[_account] = 0;
        rewardToken.safeTransfer(_account, reward);
        emit RewardPaid(_account, reward);
    }
}

So, if anyone claims directly on behalf of the strategy, the funds will get locked in the strategy contract, since the swap after the claim only accounts for balance differences within the compound call and no other function exists to handle unswapped rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

Dont check for balance difference and take the whole balance in the contract for reward tokens after the claim. Add any input validation if necessary.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

cryptotechmaker (sponsor) confirmed

[c4-judge]

dmvt marked issue #1425 as primary and marked this issue as a duplicate of 1425

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as satisfactory