search

hats-finance / hats-contracts

MIT License
36 stars 47 forks source link

hexens reaudit 6: INCORRECT COMPARISON FOR TIMESTAMP #444

Closed lirona closed 1 year ago

lirona commented 1 year ago

SEVERITY: Informational PATH: HATVault.sol:approveClaim:L273 REMEDIATION: in HATVault.sol:challengeClaim the comparison should be replaced by block.timestamp >= activeClaim.createdAt + activeClaim.challengePeriod so that activeClaim.challengedAt will be strictly smaller than activeClaim.createdAt + activeClaim.challengePeriod. STATUS: DESCRIPTION: In HATVault.sol:challengeClaim (L243-255) a claim can be challenged, unless block.timestamp > activeClaim.createdAt + activeClaim.challengePeriod is true. This means that activeClaim.challengeAt can be exactly equal to activeClaim.createdAt + activeClaim.challengePeriod. In HATVault.sol:approveClaim (L258-287) a claim can be approved, unless it is expired. A claim is considered expired in both approveClaim and dismissClaim if block.timestamp >= _claim.createdAt + _claim.challengePeriod + _claim.challengeTimeOutPeriod is true. However, in approveClaim at L273 the check to see if the claim is still in the challenge period (revert if block.timestamp > _claim.challengedAt + _claim.challengeTimeOutPeriod) includes the case where block.timestamp == _claim.createdAt + _claim.challengePeriod + _claim.challengeTimeOutPeriod. This case occurs if _claim.challengedAt == _claim.createdAt + _claim.challengePeriod, which is possible in challengeClaim, but it will never be reached because the claim would have expired.

jellegerbrandy commented 1 year ago

I am honestly not getting what is the problem here

jellegerbrandy commented 1 year ago

To summarize: the issue a small inconsistency in the challenge times. In almost all cases, the deadline to approve/dismiss is given by challengedAt + challengeTimeOutPeriod, and includes blocks with that timestamp. There is one exception, if the timestamp of the block where the claim is chellened is equal to createdAt + challengePeriod, in which case approval will revert if done on challengedAt + challengeTimeOutPeriod. The fix addresses this by simply excluding that case (of challenging in a block with a timestamp equal to createAt + challengePeriod)