When computing a vaultPortion of the prize pool over a draw, the vaultContributed and totalContributed variables are computed and returned as uint256 values. These variables are then unsafely casted to int256 which could result in a silent overflow which would result in the computed value of the vault portion being incorrect and therefore the reward dispersion also being incorrect. Arguably the original issue should have been a QA because I can’t see how the disbursed rewards for a single draw could ever get remotely close to type(uint256).max.
Mitigation
The updated implementation used OZ SafeCast to safely cast the returned uint256 values to int256. The _getVaultPortion call will now revert if the cast results in an overflow. The potential unintended consequence of this change is that any prize claims for the given vault will now revert if this cast fails, leading to claims for the vault being completely broken for the given draw. However this would be impossible in a single draw because nowhere near that much is being disbursed in a single draw. Therefore this mitigation is sound and protects against a potential overflow when querying contributions across a large number of draws.
Lines of code
Vulnerability details
Comments
When computing a
vaultPortion
of the prize pool over a draw, thevaultContributed
andtotalContributed
variables are computed and returned asuint256
values. These variables are then unsafely casted toint256
which could result in a silent overflow which would result in the computed value of the vault portion being incorrect and therefore the reward dispersion also being incorrect. Arguably the original issue should have been a QA because I can’t see how the disbursed rewards for a single draw could ever get remotely close totype(uint256).max
.Mitigation
The updated implementation used OZ SafeCast to safely cast the returned
uint256
values toint256
. The_getVaultPortion
call will now revert if the cast results in an overflow. The potential unintended consequence of this change is that any prize claims for the given vault will now revert if this cast fails, leading to claims for the vault being completely broken for the given draw. However this would be impossible in a single draw because nowhere near that much is being disbursed in a single draw. Therefore this mitigation is sound and protects against a potential overflow when querying contributions across a large number of draws.Conclusion
LGTM