QuoteHandler Manual Review Findings

QHR-01M: Improper Enforcement of Minimum Signers

Type	Severity	Location
Logical Fault		QuoteHandler.sol:L239

Description:

The LenderVaultImpl::minNumOfSigners is enforced in an equality check rather than a greater-than-or-equal check, rendering its expected purpose as "minimum" unfulfilled.

Impact:

Presently, the signature validation code can fail if more than the desired signatures are supplied which is an unexpected trait of the QuoteHandler::areValidSignatures function.

Example:

Copy
229function areValidSignatures(
230    address lenderVault,
231    bytes32 offChainQuoteHash,
232    uint8[] calldata v,
233    bytes32[] calldata r,
234    bytes32[] calldata s
235) internal view returns (bool) {
236    if (
237        v.length != r.length ||
238        v.length != s.length ||
239        v.length != ILenderVaultImpl(lenderVault).minNumOfSigners()
240    ) {
241        return false;
242    }

Recommendation:

We advise the code to ensure that v.length is greater-than-or-equal-to the value of LenderVaultImpl::minNumOfSigners, ensuring the code behaves as expected.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Myso Finance team has evaluated this exhibit and has stated that this limitation is in fact intended as they wish multi-signature validations to be as optimal as possible and thus require the exact amount of signatures necessary rather than permit a surplus.

As such, we consider this exhibit nullified given that it represents intended behaviour by the Myso Finance team.

QHR-02M: Insecure Duplicate Address Evaluations

Type	Severity	Location
Logical Fault		QuoteHandler.sol:L253, L255, L256

Description:

The code of QuoteHandler::areValidSignatures will insecurely validate signatures by using consecutive bitwise OR operations with the resulting hashes of each signatory. While the hashing operation will reduce false positives, false positives would still be present in an important module of the system.

Impact:

It is presently possible for multiple unique signatures of unique addresses to fail in the validation workflow of QuoteHandler::areValidSignatures incorrectly.

Example:

Copy
229function areValidSignatures(
230    address lenderVault,
231    bytes32 offChainQuoteHash,
232    uint8[] calldata v,
233    bytes32[] calldata r,
234    bytes32[] calldata s
235) internal view returns (bool) {
236    if (
237        v.length != r.length ||
238        v.length != s.length ||
239        v.length != ILenderVaultImpl(lenderVault).minNumOfSigners()
240    ) {
241        return false;
242    }
243    bytes32 messageHash = keccak256(
244        abi.encodePacked(
245            "\x19Ethereum Signed Message:\n32",
246            offChainQuoteHash
247        )
248    );
249    uint256 tmp;
250    address recoveredSigner;
251    uint256 newHash;
252    for (uint256 i = 0; i < v.length; ) {
253        recoveredSigner = ecrecover(messageHash, v[i], r[i], s[i]);
254        // use hash instead of address to spread out over 256 bits and reduce false positives
255        newHash = uint256(keccak256(abi.encode(recoveredSigner)));
256        if (tmp == tmp | newHash) {
257            return false;
258        }
259
260        if (!ILenderVaultImpl(lenderVault).isSigner(recoveredSigner)) {
261            return false;
262        }
263        tmp |= newHash;
264        unchecked {
265            i++;
266        }
267    }
268    return true;
269}

Recommendation:

We advise the code of LenderVaultImpl::isSigner to be revised, searching for and yielding the index of the recoveredSigner. The logic of QuoteHandler::areValidSignatures should then use this index as a bitwise shift operation of 1, setting one bit per signatory index as the recovered signers are iterated.

This approach will guarantee that there are no false positives while being as optimal as possible to ensure a unique bit index per signatory. To note, this approach will support up to 256 different signers which should be more than sufficient for the use cases of the QuoteHandler::checkAndRegisterOffChainQuote function.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The code has instead opted to apply an even more optimal approach, taking advantage of off-chain ordering of the input variables to ensure that they are in strictly ascending order and thus always unique without using a local variable structure.

As a result, we consider this exhibit fully alleviated given that false positives are no longer possible in the latest implementation.

QHR-03M: Insufficient Restriction of Loan Tenors

Type	Severity	Location
Input Sanitization		QuoteHandler.sol:L363-L364

Description:

The QuoteHandler::isValidOnChainQuote function will validate that the tenor of a loan is greater than the earliestRepayTenor, however, this delta can be as little as one second.

Impact:

Should the tenor be only one second away from the earliestRepayTenor, the repayment of the loan would be impossible in practice due to the varying block times a blockchain has as the time window the transaction would need to execute in would be 1 second.

Example:

Copy
362if (
363    onChainQuote.quoteTuples[k].tenor <=
364    onChainQuote.generalQuoteInfo.earliestRepayTenor
365) {
366    return false;
367}

Recommendation:

We advise a minimum time delta to be enforced between a loan's expiry and a loan's earliest repayment, ensuring that the purveyor of the loan has sufficient time to repay the loan under normal conditions (i.e. 24 hours).

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

A minimum delta of 24 hours has been introduced to the codebase in the form of Constants::MIN_TIME_BETWEEN_EARLIEST_REPAY_AND_EXPIRY, guaranteeing adequate time for a borrower to repay their loan and thus alleviating this exhibit.

QHR-04M: Insufficient Sanitization of Token Types

Type	Severity	Location
Input Sanitization		QuoteHandler.sol:L40-L47, L77-L84, L297-L306

Description:

The Myso Finance team has introduced the notion of compartment modules for asset classes that are inherently incompatible with the peer-to-peer pool system due to their at-rest rebasing nature (i.e. their balance can increase / decrease while they are locked in a particular account).

While the compartments adequately support these tokens, the QuoteHandler does not prohibit collaterals of such nature to be supplied "normally" without compartments. While this could be classified as a "feature", allowing a borrower to "forfeit" their yield to the lender, it can be catastrophic to the LenderVaultImpl contract's accounting system if the token can be deflationary, such as stETH in case a Lido ETH2.0 operator is slashed.

Impact:

A deflationary token would cause loans to be unable to be repaid as their collateral would be unfulfillable by the contract's balance. Additionally, proportionate distribution calculations would also misbehave as they would acquire a greater proportion than expected.

Example:

Copy
287function checkSenderAndGeneralQuoteInfo(
288    address borrower,
289    DataTypesPeerToPeer.GeneralQuoteInfo calldata generalQuoteInfo
290) internal view {
291    address _addressRegistry = addressRegistry;
292    if (
293        msg.sender != IAddressRegistry(_addressRegistry).borrowerGateway()
294    ) {
295        revert Errors.InvalidSender();
296    }
297    if (
298        IAddressRegistry(_addressRegistry).whitelistState(
299            generalQuoteInfo.collToken
300        ) !=
301        DataTypesPeerToPeer.WhitelistState.TOKEN ||
302        IAddressRegistry(_addressRegistry).whitelistState(
303            generalQuoteInfo.loanToken
304        ) !=
305        DataTypesPeerToPeer.WhitelistState.TOKEN
306    ) {
307        revert Errors.NonWhitelistedToken();
308    }

Recommendation:

We advise the WhitelistState data entry to accommodate for a new state, COMPARTMENTALIZED_TOKEN. This state should be added to any collateral which must be wrapped with a compartment as otherwise it will not behave as expected.

In turn, the referenced statements within QuoteHandler will need to update their logic to handle the new WhitelistState, ensuring that a borrowerCompartmentImplementation has been explicitly set if the token is of a COMPARTMENTALIZED_TOKEN whitelist type.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The QuoteHandler introduced a new function called QuoteHandler::_checkTokensAndCompartmentWhitelist that will properly sanitize all token type and compartment combinations, preventing tokens that require a compartment to be supplied without one as well as compartments that do not support the specified token to be valid.

As a result of these changes, we consider this exhibit fully alleviated as all quote types properly validate their token and compartment associations.

QHR-05M: Insufficient Evaluation of Quote Expiry

Type	Severity	Location
Logical Fault		QuoteHandler.sol:L287-L290

Description:

The QuoteHandler::checkSenderAndGeneralQuoteInfo function does not properly validate that a quote is still valid as it does not check its validUntil timestamp.

Impact:

A quote meant to be available for a limited time will not expire on the validUntil timestamp, causing outdated quotes to be arbitrarily exercisable.

Example:

Copy
287function checkSenderAndGeneralQuoteInfo(
288    address borrower,
289    DataTypesPeerToPeer.GeneralQuoteInfo calldata generalQuoteInfo
290) internal view {
291    address _addressRegistry = addressRegistry;
292    if (
293        msg.sender != IAddressRegistry(_addressRegistry).borrowerGateway()
294    ) {
295        revert Errors.InvalidSender();
296    }
297    if (
298        IAddressRegistry(_addressRegistry).whitelistState(
299            generalQuoteInfo.collToken
300        ) !=
301        DataTypesPeerToPeer.WhitelistState.TOKEN ||
302        IAddressRegistry(_addressRegistry).whitelistState(
303            generalQuoteInfo.loanToken
304        ) !=
305        DataTypesPeerToPeer.WhitelistState.TOKEN
306    ) {
307        revert Errors.NonWhitelistedToken();
308    }
309    if (generalQuoteInfo.collToken == generalQuoteInfo.loanToken) {
310        revert Errors.InvalidQuote();
311    }
312    if (
313        generalQuoteInfo.borrower != address(0) &&
314        generalQuoteInfo.borrower != borrower
315    ) {
316        revert Errors.InvalidBorrower();
317    }
318}
319
320function isValidOnChainQuote(
321    DataTypesPeerToPeer.OnChainQuote calldata onChainQuote
322) internal view returns (bool) {
323    if (
324        onChainQuote.generalQuoteInfo.collToken ==
325        onChainQuote.generalQuoteInfo.loanToken
326    ) {
327        return false;
328    }
329    if (onChainQuote.quoteTuples.length == 0) {
330        return false;
331    }
332    if (onChainQuote.generalQuoteInfo.validUntil < block.timestamp) {
333        return false;
334    }

Recommendation:

We advise the code to introduce a block.timestamp check, ensuring that a quote that has expired cannot be exercised if dealing with on-chain quotes.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The QuoteHandler::checkSenderAndGeneralQuoteInfo function will properly validate that the quote has not expired in the latest iteration of the codebase, rendering this exhibit alleviated.