Platform: Code4rena
Start Date: 28/01/2022
Pot Size: $30,000 USDC
Total HM: 4
Participants: 22
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 2
Id: 80
League: ETH
Rank: 5/22
Findings: 3
Award: $1,045.22
🌟 Selected for report: 2
🚀 Solo Findings: 0
sirhashalot
The Chainlink latestRoundData() function is used in Cvx3CrvOracle.sol, but it is used without checking whether the data returns from the oracle is stale or not. Chainlink warns about this issue and describes how to check for it: https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round
From Cvx3CrvOracle.sol lines 120-122
(, int256 daiPrice, , , ) = DAI.latestRoundData(); (, int256 usdcPrice, , , ) = USDC.latestRoundData(); (, int256 usdtPrice, , , ) = USDT.latestRoundData();
Only the price is retrieved from the Chainlink latestRoundData() function call. This price is accepted as valid data without any checks. This can lead to using stale prices, which can be problematic in volatile market conditions. For an example of this same issue found by an audit firm, see this finding from Consensys Diligence.
Add checks for the round ID and timestamp returned by the Chainlink latestRoundData() function. For example:
(uint80 roundID, int256 daiPrice, , uint256 timestamp, uint80 answeredInRound) = DAI.latestRoundData(); require(daiPrice > 0, "Chainlink pricefeed reporting 0"); require(answeredInRound >= roundID, "Chainlink price is stale"); require(timestamp != 0, "Chainlink round incomplete");
#0 - iamsahu
2022-02-01T19:27:36Z
#136
🌟 Selected for report: sirhashalot
95.0104 USDC - $95.01
sirhashalot
The _calcCvxIntegral() function in ConvexStakingWrapper.sol doesn't use the same gas optimization that its sibling function _calcRewardIntegral() uses.
This code is from the _calcCvxIntegral() function
if (_isClaim || userI < cvxRewardIntegral) { uint256 receiveable = cvx_claimable_reward[_accounts[u]] + ((_balances[u] * (cvxRewardIntegral - userI)) / 1e20); if (_isClaim) { if (receiveable > 0) { cvx_claimable_reward[_accounts[u]] = 0; IERC20(cvx).safeTransfer(_accounts[u], receiveable); bal = bal - (receiveable); } } else { cvx_claimable_reward[_accounts[u]] = receiveable; } cvx_reward_integral_for[_accounts[u]] = cvxRewardIntegral; }
The related code from the _calcRewardIntegral() function has the receivable calculation inside the if (_isClaim) code branch to save gas if _isClaim is false.
if (_isClaim || userI < rewardIntegral) { if (_isClaim) { uint256 receiveable = reward.claimable_reward[_accounts[u]] + ((_balances[u] * (uint256(rewardIntegral) - userI)) / 1e20); if (receiveable > 0) { reward.claimable_reward[_accounts[u]] = 0; IERC20(reward.reward_token).safeTransfer(_accounts[u], receiveable); bal = bal - receiveable; } } else { reward.claimable_reward[_accounts[u]] = reward.claimable_reward[_accounts[u]] + ((_balances[u] * (uint256(rewardIntegral) - userI)) / 1e20); } reward.reward_integral_for[_accounts[u]] = rewardIntegral; }
This optimization would save gas each time _checkpoint() is called because _checkpoint() sets _isClaim to false and doesn't enter the if(_isClaim) branch.
Modify the _calcCvxIntegral() function to place the receiveable calculation inside the if (_isClaim) code branch.
#0 - GalloDaSballo
2022-02-13T16:43:14Z
You save gas on all the operations you don't do, nice finding
🌟 Selected for report: sirhashalot
881.0898 USDC - $881.09
sirhashalot
The _calcRewardIntegral function casts intermediate reward values from uint256 to uint128 and vice versa several times. Because OpenZeppelin SafeCast is not used, casting from uint256 to uint128 may overflow if a large reward value is being calculate. This overflow could result in users receiving less rewards than they are owed.
There are 4 uint128 casting operations and 2 uint256 casting operations in the _calcRewardIntegral function of ConvexStakingWrapper.sol.
Because reward values are an important part of this protocol, use the OpenZeppelin SafeCast library to prevent unexpected overflows when casting. SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.
#0 - alcueca
2022-02-02T15:04:06Z
A better solution would be to store values in uint128 (if that is required), then cast to uint256 for calculations to gain range, and then cast with safety guards back to uint128 at the end.
#1 - GalloDaSballo
2022-02-13T23:47:30Z
I agree with the finding, personally I'd recommend the sponsor to use the higher uint256 type unless they can reliably use uint128 without castings.
That said, the warden identified castings that "could" be dangerous and in lack of any specific exploit I believe low severity to be appropriate