code423n4 commented 2 years ago

Handle

Dravee

Vulnerability details

Impact

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).

Proof of Concept

In ConvexStakingWrapper.sol, these 2 lines use the same addresses in their map:

34:     mapping(address => uint256) public cvx_reward_integral_for;
35:     mapping(address => uint256) public cvx_claimable_reward;

See the @audit-info tags in _calcCvxIntegral() for more details:

File: ConvexStakingWrapper.sol
177:             uint256 userI = cvx_reward_integral_for[_accounts[u]]; // @audit-info cvx_reward_integral_for using address _accounts[u]  
178:             if (_isClaim || userI < cvxRewardIntegral) {
179:                 uint256 receiveable = cvx_claimable_reward[_accounts[u]] + // @audit-info cvx_claimable_reward using address _accounts[u]  
180:                     ((_balances[u] * (cvxRewardIntegral - userI)) / 1e20);
181:                 if (_isClaim) {
182:                     if (receiveable > 0) {
183:                         cvx_claimable_reward[_accounts[u]] = 0; // @audit-info cvx_claimable_reward using address _accounts[u]  
184:                         IERC20(cvx).safeTransfer(_accounts[u], receiveable);
185:                         bal = bal - (receiveable);
186:                     }
187:                 } else {
188:                     cvx_claimable_reward[_accounts[u]] = receiveable; // @audit-info cvx_claimable_reward using address _accounts[u]  
189:                 }
190:                 cvx_reward_integral_for[_accounts[u]] = cvxRewardIntegral; // @audit-info cvx_reward_integral_for using address _accounts[u]  
191:             }

Tools Used

VS Code

Recommended Mitigation Steps

I'd suggest these 2 related data get grouped in a struct, let's name it CvxRewardInfo:

struct CvxRewardInfo {
  uint256 integralFor;
  uint256 claimable;
}

And it would be used as a state variable in this manner:

mapping(address => CvxRewardInfo) cvxRewardInfo;

alcueca commented 2 years ago

While true, I would consider this a code style issue, and I think we won't mess with the code to fix it.

GalloDaSballo commented 2 years ago

I think the finding has merit but it's related to coding style, as such will downgrade to non-critical

code-423n4 / 2022-01-yield-findings