Open code423n4 opened 2 years ago
Yikes, Abracadabra's code leaves a bit to be desired. We were wrong in trusting them.
@alcueca and @iamsahu are you sure the finding is valid?
From checking the code, _checkpointAndClaim
is called only once, by getReward
This does a checkpoint, where the second account is always 0, as such the check the warden mentions (second account balance) is not necessary.
The second balance is always zero as the second account is always address(0)
Can you please check my counter-argument and let me know if you actually need to have a second balance?
Images to make conversation easier:
(Second account is always 0)
So the 2nd balance being 0 is a optimization
It is true that calling _checkpointAndClaim
with two accounts would cause a loss. However, it is also true that the current code doesn't ever call checkpointAndClaim
with two accounts. Considering that, you could downgrade the severity of the bug (_checkpointAndClaim
should at least revert if passed a second account).
From my understanding, the finding originally had no impact.
While technically correct (it ignores the second account), because it was used exclusively for single accounts, no loss of funds could happen.
Given that the sponsor:
I'll downgrade to Low Severity
Handle
GeekyLumberjack
Vulnerability details
uint256[2] memory depositedBalance;
is defined at the beginning of _checkpointAndClaim only onedepositedBalance
slot is being filed and then the entire array gets passed into_calcRewardIntegral()
and_calcCvxIntegral()
along with an array of two_accounts
. Having only one of thedepositedBalance
and two_accounts
may cause loss in rewards for the second account. This function is currently only used inGetReward()
which is passing in a zero address as the second address.