AdminContract Code Style Findings

ACT-01C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		AdminContract.sol:L294, L295, L323-L324, L338-L339, L353-L354, L368, L371, L385-L386, L399-L400, L414, L417, L422, L425

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

Copy
283function setAsDefaultWithRemptionBlock(
284    address _collateral,
285    uint256 blockInDays
286)
287    external
288    onlyOwner // TODO: Review if should set to controller
289{
290    if (blockInDays > 14) {
291        blockInDays = REDEMPTION_BLOCK_DAY;
292    }
293
294    if (collateralParams[_collateral].redemptionBlock == 0) {
295        collateralParams[_collateral].redemptionBlock = block.timestamp + (blockInDays * 1 days);
296    }
297
298    _setAsDefault(_collateral);
299}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation:

All inefficient mapping lookups have been significantly optimized per our recommendation, rendering this exhibit fully alleviated.

ACT-02C: Inexistent Error Message

Type	Severity	Location
Code Style		AdminContract.sol:L144

Description:

The linked require check has no error message explicitly defined.

Example:

Copy
144require(!isInitialized);

Recommendation:

We advise one to be set so to increase the legibility of the codebase and aid in validating the require check's condition.

Alleviation:

An error message has been properly introduced to the referenced require check as advised.

ACT-03C: Loop Iterator Optimizations

Type	Severity	Location
Gas Optimization		AdminContract.sol:L214, L245

Description:

The linked for loops increment / decrement their iterator "safely" due to Solidity's built - in safe arithmetics (post-0.8.X).

Example:

Copy
214for (uint256 i = 0; i < _collaterals.length; i++) {

Recommendation:

We advise the increment / decrement operations to be performed in an unchecked code block as the last statement within each for loop to optimize their execution cost.

Alleviation:

The loop iterator increments have been optimized as advised, however, their i++ counterpart is utilized instead of ++i. We advise the latter to be set in use as it is more optimal than the present code.

ACT-04C: Misleading Variable Name

Type	Severity	Location
Code Style		AdminContract.sol:L294-L295

Description:

The redemptionBlock of the collateralParams of a given asset does not represent blocks and instead represents time as evidenced in AdminContract::setAsDefaultWithRemptionBlock and VesselManagerOperations::_validateRedemptionRequirements.

Example:

Copy
283function setAsDefaultWithRemptionBlock(
284    address _collateral,
285    uint256 blockInDays
286)
287    external
288    onlyOwner // TODO: Review if should set to controller
289{
290    if (blockInDays > 14) {
291        blockInDays = REDEMPTION_BLOCK_DAY;
292    }
293
294    if (collateralParams[_collateral].redemptionBlock == 0) {
295        collateralParams[_collateral].redemptionBlock = block.timestamp + (blockInDays * 1 days);
296    }
297
298    _setAsDefault(_collateral);
299}

Recommendation:

We advise the data point to be aptly renamed to illustrate that it represents time rather than blocks, avoiding potential confusion when reading the codebase.

Alleviation:

The redemptionBlock variable was renamed to redemptionBlockTimestamp, illustrating the variable's purpose in a clearer way.