stake.link

Summary

This code review highlights two areas for improvement: 1) optimizing setter functions by capturing both old and new values without unnecessary caching for critical parameters, and 2) removing or handling unused errors to avoid potential security vulnerabilities in contracts. Addressing these points will enhance code readability, reduce memory usage, and improve overall robustness.

Vulnerability Details

1.Optimize logic in setter functions

Capture both old and new values for critical parameter changes: When a critical parameter changes, it's essential to record both its previous value (the "old" value) and its updated value (the "new" value). This information provides a comprehensive history of the change and enables accurate tracking, analysis, and potential rollback if needed.

No need to cache the old value in this scenario: In situations where you only need to use the old value once for emitting events or other immediate actions, there's no need to create a separate cached variable to store it. This optimization can improve code readability and potentially reduce memory usage.

SDLPoolCCIPController::setMaxLINKFee

All other instances entailed:

LinearBoostController::setMaxLockingDuration

LinearBoostController::setMaxBoost

2、Unhandled Errors: InvalidParams(), DuplicateContract(), ContractNotFound()

The errors InvalidParams(), DuplicateContract(), and ContractNotFound() are not used in the SDLPool.sol contract or its child contract SDLPoolPrimary.sol and SDLPoolSecondary.sol. This is a potential issue because it means that these errors are not being handled correctly.

If one of these errors is thrown, it could indicate a problem with the contract's logic. However, because the errors are not being handled, the contract will not be able to respond to the error in a meaningful way. This could lead to unexpected behavior or even security vulnerabilities.

To fix this issue, the errors should be removed or handled to improve code security.

SDLPool.sol

Impact

Enhanced Tracking and Analysis: Optimize memory usage and enable offchain monitoring by capturing old and new values for critical parameter changes through events, avoiding unnecessary caching.
Unhandled Errors: The errors InvalidParams(), DuplicateContract(), and ContractNotFound() are defined but not used in SDLPool.sol or its child contracts, meaning potential issues might go unhandled.

Tools Used

Manual

Recommendations

Capture Old and New Values for Critical Changes by Optimized Memory Usage: Adding an event will facilitate offchain monitoring when changing system parameters. Avoiding unnecessary caching of old values can reduce memory overhead and improve code efficiency.
Address unused errors: Remove them if unnecessary for code clarity. Implement handling mechanisms if essential for security and graceful error response.