sherlock-admin4 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Firstly, I agree there are two issues:

rewardAmount is called on the rewardToken, when it should be called on rewardToken().token
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:

Getting rewardAmount on rewardToken:
- 294
- 477 (this one) - best
Calling safeTransfer on rewardContract:
- 161
- 222
- 277
- 391
- 399
- 444
- 447
- 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 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/Zivoe/zivoe-core-foundry/pull/273

panprog commented 1 month ago

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

Evert0x commented 1 month ago

Result: High Has duplicates

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status:

RealLTDingZhen: accepted

sherlock-admin2 commented 1 month ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-03-zivoe-judging

BoRonGod - Protocol unable to get extra Rewards in OCY_Convex_C #477

Protocol unable to get extra Rewards in OCY_Convex_C

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

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.

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

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.

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

294

477 (this one) - best

161

222

277

391

399

444

447

479 - best

sherlock-audit / 2024-03-zivoe-judging

BoRonGod - Protocol unable to get extra Rewards in OCY_Convex_C #477

Protocol unable to get extra Rewards in OCY_Convex_C

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

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.

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

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.

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

294

477 (this one) - best

161

222

277

391

399

444

447

479 - best

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 happens on different lines in `OCY_Convex_C`, with different external calls.

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 happens on different lines in `OCY_Convex_C`, with different external calls.