LenderVaultImpl Code Style Findings

LVI-01C: Code Repetition

Type	Severity	Location
Gas Optimization		LenderVaultImpl.sol:L59-L62

Description:

The referenced statements are meant to replicate the Ownable::senderCheckOwner implementation.

Example:

Copy
59// only owner can call this function
60if (msg.sender != _owner) {
61    revert Errors.InvalidSender();
62}

Recommendation:

We advise the Ownable::senderCheckOwner function to be invoked in the statements' place, ensuring consistency throughout the codebase.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The referenced code block was properly replaced by the now-renamed Ownable::_senderCheckOwner function, optimizing the code's maintainability.

LVI-02C: Implicit Arithmetic Grouping

Type	Severity	Location
Code Style		LenderVaultImpl.sol:L84-L85

Description:

The referenced calculations are meant to be performed before the subtraction, however, they are not wrapped in parenthesis (()) to explicitly mark this behaviour.

Example:

Copy
82tmp =
83    _loan.initCollAmount -
84    (_loan.initCollAmount * _loan.amountRepaidSoFar) /
85    _loan.initRepayAmount;

Recommendation:

We advise them to be wrapped in parenthesis as currently Solidity's arithmetic order of precedence takes place which may not be immediately apparent to the code's readers.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The arithmetic operation was revised to properly illustrate its expected order of precedence and to utilize a single fraction, optimizing its legibility.

LVI-03C: Inefficient Removal of Signer

Type	Severity	Location
Gas Optimization		LenderVaultImpl.sol:L281-L282

Description:

The LenderVaultImpl::removeSigner function will inefficiently remove a signer if they exist at the end of the signers array.

Example:

Copy
271function removeSigner(address signer, uint256 signerIdx) external {
272    senderCheckOwner();
273    uint256 signersLen = signers.length;
274    if (signerIdx >= signersLen) {
275        revert Errors.InvalidArrayIndex();
276    }
277
278    if (!isSigner[signer] || signer != signers[signerIdx]) {
279        revert Errors.InvalidSignerRemoveInfo();
280    }
281    address signerMovedFromEnd = signers[signersLen - 1];
282    signers[signerIdx] = signerMovedFromEnd;
283    signers.pop();
284    isSigner[signer] = false;
285    emit RemovedSigner(signer, signerIdx, signerMovedFromEnd);
286}

Recommendation:

We advise the code to perform the signer replacement with the last entry solely when signersLen - 1 != signerIdx as in such a case a simple Array::pop operation is sufficient.

Alleviation (c740f7c6b5):

The code was revised to replace the signerIdx member solely when necessary, however, the RemovedSigner event that is emitted will mistakenly specify that the signer moved from the end is the signer that is being removed in the signerIdx == signersLen - 1 scenario. We advise the signerMovedFromEnd to be 0 in such a case, further optimizing the code to perform one less SLOAD operation when a replacement is not necessary.

Alleviation (37cf23668b):

The code was further optimized as advised, relocating the signerMovedFromEnd assignment (now aptly renamed to signerWithSwappedPosition) within the if block thus rendering this exhibit's optimization applied in full.

LVI-04C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		LenderVaultImpl.sol:L94, L103

Description:

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

Example:

Copy
94lockedAmounts[collToken] -= totalUnlockableColl;
95// if collToken is not used by vault as loan token, then
96// vault owner may have wanted to leave unlocked coll in vault
97if (autoWithdraw) {
98    uint256 currentCollTokenBalance = IERC20Metadata(collToken)
99        .balanceOf(address(this));
100
101    IERC20Metadata(collToken).safeTransfer(
102        _owner,
103        currentCollTokenBalance - lockedAmounts[collToken]
104    );
105}

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 (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The referenced optimization is no longer applicable as the autoWithdraw feature has been removed from the LenderVaultImpl::unlockCollateral function, rendering this exhibit nullified.

LVI-05C: Redundant Temporary Variable

Type	Severity	Location
Gas Optimization		LenderVaultImpl.sol:L82-L85

Description:

The tmp variable declared within the for loop of LenderVaultImpl::unlockCollateral is redundant as it is utilized solely in a single calculation which is then added to the outer-level totalUnlockableColl variable.

Example:

Copy
67uint256 totalUnlockableColl;
68for (uint256 i = 0; i < _loanIds.length; ) {
69    uint256 tmp = 0;
70    DataTypesPeerToPeer.Loan storage _loan = _loans[_loanIds[i]];
71
72    if (_loan.collToken != collToken) {
73        revert Errors.InconsistentUnlockTokenAddresses();
74    }
75    if (_loan.collUnlocked || block.timestamp < _loan.expiry) {
76        revert Errors.InvalidCollUnlock();
77    }
78    if (_loan.collTokenCompartmentAddr != address(0)) {
79        IBaseCompartment(_loan.collTokenCompartmentAddr)
80            .unlockCollToVault(_loan.collToken);
81    } else {
82        tmp =
83            _loan.initCollAmount -
84            (_loan.initCollAmount * _loan.amountRepaidSoFar) /
85            _loan.initRepayAmount;
86        totalUnlockableColl += tmp;
87    }
88    _loan.collUnlocked = true;
89    unchecked {
90        i++;
91    }
92}

Recommendation:

We advise the variable to be omitted, optimizing the loop's execution cost.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The redundant tmp variable has been safely omitted from the codebase as advised.

LVI-06C: Restrictive Automated Withdrawal Feature

Type	Severity	Location
Code Style		LenderVaultImpl.sol:L97

Description:

The LenderVaultImpl::unlockCollateral function will permit the collateral of loans to be acquired if they have expired and supports an autoWithdraw feature meant to withdraw the collateral that was withdrawn. The issue with the current design is that it either withdraws all available collateral (regardless of whether it was unlocked in the loan claim process) or no collateral at all.

In practice, lenders would want to simply withdraw the collateral they acquired from the loans that expired and not the collateral they had already deposited to the contract up for consumption by new loans.

Example:

Copy
54function unlockCollateral(
55    address collToken,
56    uint256[] calldata _loanIds,
57    bool autoWithdraw
58) external {
59    // only owner can call this function
60    if (msg.sender != _owner) {
61        revert Errors.InvalidSender();
62    }
63    // if empty array is passed, revert
64    if (_loanIds.length == 0) {
65        revert Errors.InvalidArrayLength();
66    }
67    uint256 totalUnlockableColl;
68    for (uint256 i = 0; i < _loanIds.length; ) {
69        uint256 tmp = 0;
70        DataTypesPeerToPeer.Loan storage _loan = _loans[_loanIds[i]];
71
72        if (_loan.collToken != collToken) {
73            revert Errors.InconsistentUnlockTokenAddresses();
74        }
75        if (_loan.collUnlocked || block.timestamp < _loan.expiry) {
76            revert Errors.InvalidCollUnlock();
77        }
78        if (_loan.collTokenCompartmentAddr != address(0)) {
79            IBaseCompartment(_loan.collTokenCompartmentAddr)
80                .unlockCollToVault(_loan.collToken);
81        } else {
82            tmp =
83                _loan.initCollAmount -
84                (_loan.initCollAmount * _loan.amountRepaidSoFar) /
85                _loan.initRepayAmount;
86            totalUnlockableColl += tmp;
87        }
88        _loan.collUnlocked = true;
89        unchecked {
90            i++;
91        }
92    }
93
94    lockedAmounts[collToken] -= totalUnlockableColl;
95    // if collToken is not used by vault as loan token, then
96    // vault owner may have wanted to leave unlocked coll in vault
97    if (autoWithdraw) {
98        uint256 currentCollTokenBalance = IERC20Metadata(collToken)
99            .balanceOf(address(this));
100
101        IERC20Metadata(collToken).safeTransfer(
102            _owner,
103            currentCollTokenBalance - lockedAmounts[collToken]
104        );
105    }
106
107    emit CollateralUnlocked(_owner, collToken, _loanIds, autoWithdraw);
108}

Recommendation:

We advise the functionality of LenderVaultImpl::unlockCollateral to be increased, yielding the totalUnlockableColl value to the _owner in case autoWithdraw was set to true.

To ensure compatibility with compartments, the IBaseCompartment::unlockCollToVault function would need to be updated and accept a recipient input argument. If autoWithdraw is false, the recipient should be the LenderVaultImpl itself. If it is set to true, the recipient should be set to _owner.

These adjustments would permit the autoWithdraw flag to behave as expected and withdraw only the funds that were claimed from the input loans rather than "any" funds that are at rest of the specified collateral type.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Myso Finance team considered this exhibit and has opted to instead eliminate the autoWithdraw feature from the LenderVaultImpl::unlockCollateral function entirely as the complexity it introduces is not outweighed by the ease-of-use by Myso Finance's users.

As such, we consider this exhibit indirectly alleviated.