CollSurplusPool Code Style Findings

CSP-01C: Inefficient Renunciation of Ownership

Type	Severity	Location
Gas Optimization		CollSurplusPool.sol:L48

Description:

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

Example:

Copy
32function setAddresses(
33    address _activePoolAddress,
34    address _borrowerOperationsAddress,
35    address _vesselManagerAddress,
36    address _vesselManagerOperationsAddress
37) external override initializer {
38    require(!isInitialized, "Already initialized");
39    isInitialized = true;
40
41    __Ownable_init();
42
43    activePoolAddress = _activePoolAddress;
44    borrowerOperationsAddress = _borrowerOperationsAddress;
45    vesselManagerAddress = _vesselManagerAddress;
46    vesselManagerOperationsAddress = _vesselManagerOperationsAddress;
47
48    renounceOwnership();
49}

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.

CSP-02C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		CollSurplusPool.sol:L70, L71, L78, L80, L84

Description:

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

Example:

Copy
63function accountSurplus(
64    address _asset,
65    address _account,
66    uint256 _amount
67) external override {
68    _requireCallerIsVesselManager();
69
70    uint256 newAmount = userBalances[_account][_asset].add(_amount);
71    userBalances[_account][_asset] = newAmount;
72
73    emit CollBalanceUpdated(_account, newAmount);
74}

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.

CSP-03C: Inexplicable Ownable Pattern

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

Description:

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

Example:

Copy
32function setAddresses(
33    address _activePoolAddress,
34    address _borrowerOperationsAddress,
35    address _vesselManagerAddress,
36    address _vesselManagerOperationsAddress
37) external override initializer {
38    require(!isInitialized, "Already initialized");
39    isInitialized = true;
40
41    __Ownable_init();
42
43    activePoolAddress = _activePoolAddress;
44    borrowerOperationsAddress = _borrowerOperationsAddress;
45    vesselManagerAddress = _vesselManagerAddress;
46    vesselManagerOperationsAddress = _vesselManagerOperationsAddress;
47
48    renounceOwnership();
49}

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 CollSurplusPool. To properly alleviate this exhibit, we advise the OwnableUpgradeable contract to be omitted from the CollSurplusPool entirely.

CSP-04C: Redundant Initialization Paradigm

Type	Severity	Location
Gas Optimization		CollSurplusPool.sol:L23, L37-L39

Description:

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

Example:

Copy
23bool public isInitialized;
24
25// deposited ether tracker
26mapping(address => uint256) balances;
27// Collateral surplus claimable by vessel owners
28mapping(address => mapping(address => uint256)) internal userBalances;
29
30// --- Contract setters ---
31
32function setAddresses(
33    address _activePoolAddress,
34    address _borrowerOperationsAddress,
35    address _vesselManagerAddress,
36    address _vesselManagerOperationsAddress
37) external override initializer {
38    require(!isInitialized, "Already initialized");
39    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.