Open code423n4 opened 2 years ago
Duplicate: https://github.com/code-423n4/2022-09-artgobblers-findings/issues/372 I think the finding is lower severity as I stated here (second paragraph): https://github.com/code-423n4/2022-09-artgobblers-findings/issues/372#issuecomment-1260684497
I think that the specific issue is:
gobblerRevealsData.nextRevealTimestamp
will be in the pastThis property of the system is shown via this test
/// @notice Can request earlier if we are behind scheduled
function testCanRevealFasterIfLate() public {
// Mint and reveal first gobbler
mintGobblerToAddress(users[0], 1);
vm.warp(block.timestamp + 48 days);
setRandomnessAndReveal(1, "seed");
// Attempt reveal before 24 hours have passed
mintGobblerToAddress(users[0], 1);
// Works because we are 47 days late
gobblers.requestRandomSeed();
}
Which in contrast to testRevealDelayRecurring
doesn't revert, because we are behind schedule.
The finding is Valid, I'm not convinced by the Medium Severity, Low is definitely correct as the expectation from code, comment and test is broken, however Medium seems like a stretch as arguably this allows revealing more gobblers when the system is late, not during the expected 24 hour windows.
In other words, the reveals are still every 24 hours, but they are 24 hours since launch, not 24 hours since the previous reveal.
In a way this is better behaviour than waiting 24 hours "if we're fallen behind". I also think the finding hasn't shown any meaningful economic loss nor availability of service, if anything we're getting reveals relatively faster in this situation.
Given the information that I have, I think the finding is valid but I'll downgrade to Low as no loss was shown
While expectations have been broken, they seem to gracefully degrade toward a better outcome (faster reveal when behind schedule), instead of forcing the end user to wait. This without any demonstrated leak of value nor exploit.
For those reasons I'll downgrade to Low
L
Lines of code
https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L509-L542
Vulnerability details
Impact
requestRandomSeed
should be called only once in 24 hours. But it can be called in less than 24 hours if the previous reveal timestamp doesn't exactly match with thenextRevealTimestamp
.Proof of Concept
Looking at the requestRandomSeed function:
But the mistake lies in how the
nextRevealTimestamp
is set in the function.Note that we are adding
1 day
to the previousnextRevealTimestamp
. But if the currentblock.timestamp
doesn't coincide with the previousnextRevealTimestamp
, then logically it means:(
next nextRevealTimestamp
-current block.timestamp
) <1 day
For example if the current
nextRevealTimestamp
is 8:30 am today. And if someone calls therequestRandomSeed
function at 12:30 pm, then the futurenextRevealTimestamp
will be set at 8:30 am tomorrow instead of 12:30 pm tomorrow. Which means theseed
could be requested only after a delay of 20 hours.Tools Used
Manual analysis, VSCode
Recommended Mitigation Steps
Change line 535 to as shown below. Basically we use
block.timestamp
instead of previousnextRevealTimestamp
in the calculation.