Zero token transfer can cause a potential DoS in CVXStaker

mediumCode4rena

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L185-L198

Vulnerability details

The CVXStaker contract doesn't check for zero amount while transferring rewards, which can end up blocking the operation.

Impact

The CVXStaker contract is in charge of handling interaction with the Convex pool. The getReward() function is used to claim rewards and transfer them to the rewards recipient:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L185-L198

solidity📋
185:     function getReward(bool claimExtras) external {
186:         IBaseRewardPool(cvxPoolInfo.rewards).getReward(
187:             address(this),
188:             claimExtras
189:         );
190:         if (rewardsRecipient != address(0)) {
191:             for (uint i = 0; i < rewardTokens.length; i++) {
192:                 uint256 balance = IERC20(rewardTokens[i]).balanceOf(
193:                     address(this)
194:                 );
195:                 IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
196:             }
197:         }
198:     }

As we can see in the previous snippet of code, the implementation will loop all the configured reward tokens and transfer them one by one to the reward recipient.

This is a bit concerning as some ERC20 implementations revert on zero value transfers (see https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers). If at least one of the reward tokens includes this behavior, then the current implementation may cause a denial of service, as a zero amount transfer in this token will block the whole action and revert the transaction.

Note that the rewards array is not modifiable, it is defined at construction time, a token cannot be removed.

Proof of concept

We reproduce the issue in the following test. token1 is a normal ERC20 and token2 reverts on zero transfer amounts. Rewards from token1 can't be transferred to the recipient as the zero transfer on token2 will revert the operation.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

solidity📋
function test_CVXStaker_RevertOnZeroTokenTransfer() public {
    MockErc20 token1 = new MockErc20("Token1", "TOK1", 18);
    MockErc20 token2 = new RevertOnZeroErc20("Token2", "TOK2", 18);

    MockCVXRewards rewards = new MockCVXRewards();

    address operator = makeAddr("operator");
    IERC20 clpToken = IERC20(makeAddr("clpToken"));
    ICVXBooster booster = ICVXBooster(makeAddr("booster"));
    address[] memory rewardTokens = new address[](2);
    rewardTokens[0] = address(token1);
    rewardTokens[1] = address(token2);

    CVXStaker staker = new CVXStaker(operator, clpToken, booster, rewardTokens);
    staker.setCvxPoolInfo(0, address(clpToken), address(rewards));

    address rewardsRecipient = makeAddr("rewardsRecipient");
    staker.setRewardsRecipient(rewardsRecipient);

    // simulate staker has some token1 but zero token2 after calling getRewards
    deal(address(token1), address(staker), 1e18);

    // The transaction will fail as the implementation will try to transfer zero
    // tokens for token2, blocking the whole operation.
    vm.expectRevert("cannot transfer zero amount");
    staker.getReward(true);
}

Recommendation

Check for zero amount before executing the transfer:

solidity📋
function getReward(bool claimExtras) external {
    IBaseRewardPool(cvxPoolInfo.rewards).getReward(
        address(this),
        claimExtras
    );
    if (rewardsRecipient != address(0)) {
        for (uint i = 0; i < rewardTokens.length; i++) {
            uint256 balance = IERC20(rewardTokens[i]).balanceOf(
                address(this)
            );
            
+           if (balance > 0) {
              IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
+           }
        }
    }
}

Assessed type

ERC20