SortedVessels Code Style Findings

SVS-01C: Inefficient Renunciation of Ownership

Type	Severity	Location
Gas Optimization		SortedVessels.sol:L48

Description:

The SortedVessels::setParams function will invoke the OwnableUpgradeable::renounceOwnership function which in turn will apply the onlyOwner modifier redundantly.

Example:

Copy
77function setParams(address _vesselManagerAddress, address _borrowerOperationsAddress)
78    external
79    override
80    initializer
81{
82    require(!isInitialized, "Already initialized");
83    isInitialized = true;
84
85    __Ownable_init();
86
87    vesselManager = IVesselManager(_vesselManagerAddress);
88    borrowerOperationsAddress = _borrowerOperationsAddress;
89
90    renounceOwnership();
91}

Recommendation:

We advise the OwnableUpgradeable::_transferOwnership function to be utilized directly, transferring ownership to the zero address.

Alleviation:

Ownership of the contract is no longer renounced in the latest iteration of the codebase rendering this exhibit inapplicable.

SVS-02C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		SortedVessels.sol:L143, L147-L148, L151-L153, L156-L158, L161-L164, L167, L184, L186, L189, L191, L195, L197, L201-L203, L205-L207, L212-L213, L216-L217, L370, L376, L383-L384, L404, L410, L417-L418

Description:

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

Example:

Copy
143data[_asset].nodes[_id].exists = true;
144
145if (prevId == address(0) && nextId == address(0)) {
146    // Insert as head and tail
147    data[_asset].head = _id;
148    data[_asset].tail = _id;
149} else if (prevId == address(0)) {
150    // Insert before `prevId` as the head
151    data[_asset].nodes[_id].nextId = data[_asset].head;
152    data[_asset].nodes[data[_asset].head].prevId = _id;
153    data[_asset].head = _id;
154} else if (nextId == address(0)) {
155    // Insert after `nextId` as the tail
156    data[_asset].nodes[_id].prevId = data[_asset].tail;
157    data[_asset].nodes[data[_asset].tail].nextId = _id;
158    data[_asset].tail = _id;
159} else {
160    // Insert at insert position between `prevId` and `nextId`
161    data[_asset].nodes[_id].nextId = nextId;
162    data[_asset].nodes[_id].prevId = prevId;
163    data[_asset].nodes[prevId].nextId = _id;
164    data[_asset].nodes[nextId].prevId = _id;
165}
166
167data[_asset].size = data[_asset].size.add(1);

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.

SVS-03C: Inexplicable Ownable Pattern

Type	Severity	Location
Gas Optimization		SortedVessels.sol:L41, L48

Description:

The SortedVessels inherits the OwnableUpgradeable implementation redundantly as it initializes it within the SortedVessels::setParams function and consequently renounces ownership in the same call.

Example:

Copy
77function setParams(address _vesselManagerAddress, address _borrowerOperationsAddress)
78    external
79    override
80    initializer
81{
82    require(!isInitialized, "Already initialized");
83    isInitialized = true;
84
85    __Ownable_init();
86
87    vesselManager = IVesselManager(_vesselManagerAddress);
88    borrowerOperationsAddress = _borrowerOperationsAddress;
89
90    renounceOwnership();
91}

Recommendation:

We advise it to be removed, inheriting the Initializable implementation of OpenZeppelin instead which is properly put in use within the contract.

Alleviation:

While the renunciation has been removed, the OwnableUpgradeable contract is still inherited by the SortedVessels. To properly alleviate this exhibit, we advise the OwnableUpgradeable contract to be omitted from the SortedVessels entirely.

SVS-04C: Redundant Initialization Paradigm

Type	Severity	Location
Gas Optimization		SortedVessels.sol:L80, L82-L83

Description:

The SortedVessels contract inherits the OpenZeppelin OwnableUpgradeable implementation which contains the Initializable implementation, put in use within the SortedVessels::setParams function. As such, the manual isInitialized flag is redundant.

Example:

Copy
77function setParams(address _vesselManagerAddress, address _borrowerOperationsAddress)
78    external
79    override
80    initializer
81{
82    require(!isInitialized, "Already initialized");
83    isInitialized = true;

Recommendation:

We advise it and its validations to be omitted from the codebase as it is ineffectual and duplicates the purpose of the Initializable::initializer modifier.

Alleviation:

The manual initialization methodology has been removed from the contract as advised.