Hawk High

Summary

The giveReview function penalizes students with a -10 score deduction for each bad review. While Solidity 0.8+ prevents numerical underflows by default (reverting on negative values), this design relies entirely on that safety mechanism instead of handling the boundary case explicitly. As a result, edge cases such as a student already at 0 receiving another bad review will trigger a revert, leading to ungraceful failure, poor user experience, and hidden fragility in future upgrades.

Vulnerability Details

This code assumes:

• reviewCount[_student] is hard-capped at 4.

• A student can never go below 0 because they won't get more than four bad reviews.

However, this assumption is not future-proof:

• If reviewCount is increased in a future upgrade (e.g., to 6 or 8), students could accumulate enough bad reviews to trigger underflows.

• Even under the current design, a student at 0 score will trigger a revert on further negative reviews, resulting in unnecessary transaction failure and gas wastage.

Example Scenario

1. Student A starts with a score of 10.

2. They receive two bad reviews:

   • After first: score = 0

   • On second: transaction reverts (0 - 10), failing silently to the end user.

3. Teacher must now handle this off-chain, possibly retry or escalate — poor UX.

Recommended Fix

Make score deduction explicitly bounded:

Optional Enhancements:

Key Improvements

Why This Matters

• User Experience: Prevents failed reviews from reverting, especially when stakes (e.g., upgrades, certificates) are tied to score outcomes.

• Defensive Programming: Resilience against logic drift and developer error in future versions.

• Protocol Predictability: Clear scoring semantics make testing and audits easier.

Recommendations

1. ✅ Bound Reductions: Explicitly cap scores at 0.

2. 🔍 Emit Edge Case Events: E.g., ScoreFloorReached for monitoring and analytics.

3. 📝 Document Score Bounds: Use NatSpec to explain minimum and maximum score behavior.

4. 🧪 Test Score Floors: Unit tests should assert that score does not go negative and does not revert at 0.

Conclusion

While Solidity’s automatic revert on underflow prevents a critical failure, relying on it to control game logic is fragile and opaque. By explicitly capping scores at zero, the protocol gains clarity, safety, and better UX, without introducing significant overhead.

Would you like this bundled into a full audit issue list alongside the storage gap and review misalignment findings?