BoRonGod - Protocol unable to get extra Rewards in OCY_Convex_C

[sherlock-admin4]

BoRonGod

high

Protocol unable to get extra Rewards in OCY_Convex_C

Summary

Convex would wrap rewardToken for pools with IDs 151+, but the counting logic in OCY_Convex_C.sol makes it impossible for zivoe to forward yield.

Vulnerability Detail

In OCY_Convex_C.sol , A convex pool with id 270 is used:

/// @dev Convex information.
address public convexDeposit = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
address public convexPoolToken = 0x383E6b4437b59fff47B619CBA855CA29342A8559;
address public convexRewards = 0xc583e81bB36A1F620A804D8AF642B63b0ceEb5c0;

uint256 public convexPoolID = 270;

In the following logic, rewardContract is defaulted to the address of extraRewards. This assumption is fine for pools with PoolId < 150, but would not work for IDs 151+.

/// @notice Claims rewards and forward them to the OCT_YDL.
/// @param extra Flag for claiming extra rewards.
function claimRewards(bool extra) public nonReentrant {
    IBaseRewardPool_OCY_Convex_C(convexRewards).getReward();

    // Native Rewards (CRV, CVX)
    uint256 rewardsCRV = IERC20(CRV).balanceOf(address(this));
    uint256 rewardsCVX = IERC20(CVX).balanceOf(address(this));
    if (rewardsCRV > 0) { IERC20(CRV).safeTransfer(OCT_YDL, rewardsCRV); }
    if (rewardsCVX > 0) { IERC20(CVX).safeTransfer(OCT_YDL, rewardsCVX); }

    // Extra Rewards
    if (extra) {
        uint256 extraRewardsLength = IBaseRewardPool_OCY_Convex_C(convexRewards).extraRewardsLength();
        for (uint256 i = 0; i < extraRewardsLength; i++) {
            address rewardContract = IBaseRewardPool_OCY_Convex_C(convexRewards).extraRewards(i); 
            //@Audit incorrect here!
            uint256 rewardAmount = IBaseRewardPool_OCY_Convex_C(rewardContract).rewardToken().balanceOf(address(this));
            if (rewardAmount > 0) { IERC20(rewardContract).safeTransfer(OCT_YDL, rewardAmount); }
        }
    }
}

According to convex doc,

for pools with IDs 151+:
VirtualBalanceRewardPool's rewardToken points to a wrapped version of the underlying token. This Token implementation can be found here: https://github.com/convex-eth/platform/blob/main/contracts/contracts/StashTokenWrapper.sol

Just check convexRewards of pool 270:

https://etherscan.io/address/0xc583e81bB36A1F620A804D8AF642B63b0ceEb5c0#readContract#F5

For index 0, it returns a VirtualBalanceRewardPool with rewardtoken = 0x85D81Ee851D36423A5784CD3Cb6f1a1193Cb5978. This contract is a StashTokenWrapper, which is consistent with what the convex documentation says.

And, when IBaseRewardPool_OCY_Convex_C(convexRewards).getReward(); is triggered, reward tokens will be unwrapped and send to caller, so rewardAmount will always return 0, means such yield cannot be claimed for zivoe.

            address rewardContract = IBaseRewardPool_OCY_Convex_C(convexRewards).extraRewards(i); 

            //@Audit incorrect here! `rewardToken` is a `StashTokenWrapper`, not the reward token!

            uint256 rewardAmount = IBaseRewardPool_OCY_Convex_C(rewardContract).rewardToken().balanceOf(address(this));
            if (rewardAmount > 0) { IERC20(rewardContract).safeTransfer(OCT_YDL, rewardAmount); }

Impact

Users will lose extra rewards from convex pools with IDs 151+.

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/lockers/OCY/OCY_Convex_C.sol#L210-L219
https://docs.convexfinance.com/convexfinanceintegration/baserewardpool

Tool used

Manual Review

Recommendation

Change the logic above to:

            uint256 rewardAmount = IBaseRewardPool_OCY_Convex_C(rewardContract).rewardToken().token().balanceOf(address(this));
            if (rewardAmount > 0) { IERC20(rewardContract).safeTransfer(OCT_YDL, rewardAmount); }

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

high, loss of additional reward from convex. Since it accumulates over 30-days period, the loss will be significant. Both balanceOf and safeTransfer are incorrect - they should be called on rewardToken().token(). Different dups of this mention either balanceOf or safeTransfer, #294 mentions both (but this one is more detailed), but I consider them to be the same root cause, so all are dups.

[RealLTDingZhen]

escalate

I would escalate this to a solo issue.

Lets compare this one with #479 (and all current dups):

#477 only works with convex pool id 151+ , so only OCY_Convex_C is affected.
#479 works with ALL three OCY_Convex.

#477 points out a incorrect external call with convex's StashTokenWrapper.
#479 points out a incorrect external call with convex's VirtualBalanceRewardPool.

#477 and #479 have different fixes. The fix for one cannot fix the other.

#477 and #479 have different impacts.

• If we fix 479 , In 477 , claimRewards will still not forward any extrareward to YDL.
• If we fix 477 , In 479 , claimRewards will still always revert.

#477 and #479 happens on different lines in OCY_Convex_C, with different external calls.

//#477 root cause: convex would wrap rewards into a StashTokenWrapper
uint256 rewardAmount = IBaseRewardPool_OCY_Convex_C(rewardContract).rewardToken().balanceOf(address(this));

//#479 and all dups root cause: convex rewardContract is a VirtualBalanceRewardPool
if (rewardAmount > 0) { IERC20(rewardContract).safeTransfer(OCT_YDL, rewardAmount); }

So, why should we duplicate two valid issues when they have different root causes, different fixes, different impacts, and the root cause of both issues do not have any code reuse?

I believe Lead judge's statement

The core reason is still incorrect address for these functions.

should not be the reason to duplicate this issue. These two different incorrect addresses are obtained through different improper external calls towards different external contracts. I don't understand why this issue was considered as dup of #479.

[sherlock-admin3]

escalate

I would escalate this to a solo issue.

Lets compare this one with #479 (and all current dups):

#477 only works with convex pool id 151+ , so only OCY_Convex_C is affected.
#479 works with ALL three OCY_Convex.

#477 points out a incorrect external call with convex's StashTokenWrapper.
#479 points out a incorrect external call with convex's VirtualBalanceRewardPool.

#477 and #479 have different fixes. The fix for one cannot fix the other.

#477 and #479 have different impacts.

• If we fix 479 , In 477 , claimRewards will still not forward any extrareward to YDL.
• If we fix 477 , In 479 , claimRewards will still always revert.

#477 and #479 happens on different lines in OCY_Convex_C, with different external calls.

//#477 root cause: convex would wrap rewards into a StashTokenWrapper
uint256 rewardAmount = IBaseRewardPool_OCY_Convex_C(rewardContract).rewardToken().balanceOf(address(this));

//#479 and all dups root cause: convex rewardContract is a VirtualBalanceRewardPool
if (rewardAmount > 0) { IERC20(rewardContract).safeTransfer(OCT_YDL, rewardAmount); }

So, why should we duplicate two valid issues when they have different root causes, different fixes, different impacts, and the root cause of both issues do not have any code reuse?

I believe Lead judge's statement

The core reason is still incorrect address for these functions.

should not be the reason to duplicate this issue. These two different incorrect addresses are obtained through different improper external calls towards different external contracts. I don't understand why this issue was considered as dup of #479.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[panprog]

The core reason is still incorrect address for these functions. The fix is different for different contracts, some issues describe it in more details, some in less. Since balanceOf and transfer have to happen on the same address, I don't think separate issues for balanceOf or transfer warrant it being a separate group. The same with _A or _C - it's basically the same, even if the fix in each of them is different.
I agree that your issue describes everything much better and the recommendation is more correct compared to dups, but still the core reason is the same. If these nuances warrant a separate group - this will have to be decided by Sherlock. I keep my decision here.

As for the severity: DAO can't pull these tokens, because the only function to pull tokens is overriden for this locker and only allows to pull convex pool token and nothing else.

[pseudonaut]

I'm confused by the final recommendation for fixes here, can someone provide detailed explanation of fixes for appropriate contracts based on all findings?

[panprog]

I'm confused by the final recommendation for fixes here, can someone provide detailed explanation of fixes for appropriate contracts based on all findings?

@pseudonaut , the address of the extra reward token is:

• for convex pools < 151 (OCY_Convex_A): address = IBaseRewardPool_OCY_Convex_A(rewardContract).rewardToken()
• for convex pools >= 151 (OCY_Convex_C): address = IBaseRewardPool_OCY_Convex_A(rewardContract).rewardToken().token() (you'll need to add token() in the reward token interface)

Both balanceOf and safeTransfer should be done on this address. Something like

        if (extra) {
            uint256 extraRewardsLength = IBaseRewardPool_OCY_Convex_A(convexRewards).extraRewardsLength();
            for (uint256 i = 0; i < extraRewardsLength; i++) {
                address rewardContract = IBaseRewardPool_OCY_Convex_A(convexRewards).extraRewards(i);
                IERC20 rewardToken = convexPoolID < 151 ? IBaseRewardPool_OCY_Convex_A(rewardContract).rewardToken() : IBaseRewardPool_OCY_Convex_A(rewardContract).rewardToken().token();
                uint256 rewardAmount = rewardToken.balanceOf(
                    address(this)
                );
                if (rewardAmount > 0) { IERC20(rewardToken).safeTransfer(OCT_YDL, rewardAmount); }
            }
        }

[WangSecurity]

Firstly, I agree there are two issues:

1. rewardAmount is called on the rewardToken, when it should be called on rewardToken().token
2. safeTransfer is called on rewardContract when it should be called on rewardToken.

Fixing one, doesn't fix the other. I agree that general core issue is in fact using the wrong address, but the fixes and impacts are different (for 1 is not paying out the extra rewards and they're kept in the contract, for 2 is the revert of the call). Hence, I believe they should be treated separately. There is only 1 report that describes both of these vulnerabilities, which is #294, therefore, I believe it would be fair to make the following issue families:

1. Getting rewardAmount on rewardToken:

• cergyk - OCY_Convex_C::claimRewards will trap extra rewards in the contract instead of transferring them to OCT_YDL #294
• BoRonGod - Protocol unable to get extra Rewards in OCY_Convex_C #477 (this one) - best

2. Calling safeTransfer on rewardContract:

• AllTooWell - claimRewards(true) in OCC_Convex_A and OCC_Convex_C will always revert because of wrong integration about extra rewards in convex #161
• 0xpiken - claimRewards() might revert due to Incorrect token transferring #222
• Drynooo - claimRewards may fail to execute #277
• ether_sky - There is an error in the claimRewards function within the OCY_Convex_A and OCY_Convex_C contracts #391
• Ironsidesec - OCY_Convex_C.claimRewards will revert if extra rewards > 0 #399
• den_sosnovskyi - In OCY_Convex_A::claimRewards rewardContract is used as ERC20, but it is not ECR20 which holds rewards #444
• Ironsidesec - claimRewards action on OCY_Convex_A will revert on extra rewards > 0 #447
• BoRonGod - cannot forward extra rewards from both OCY_Convex to OCT_YDL. #479 - best

The reason why #294 is in the 1st family is because it mentions both issues, but since one report cannot get 2 rewards, it'll be fair to include it in the family with less reports.

Lastly, I believe in both situations there are no extensive external limitations leading to a loss of funds. Please correct me if any assumption above is wrong.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
Zivoe/zivoe-core-foundry#273

[panprog]

@WangSecurity
Agree with your decision. Keeping #294 a dup of this also looks correct.

[Evert0x]

Result:
High
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• RealLTDingZhen: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.