RCN Diaspore Audit Report

Our audit report on Diaspore

Summary

We conducted a security review of RCN’s Diaspore contracts on version: c439b94851d3a

Audit criteria

The Diaspore system has two core contracts: DebtEngine and LoanManager. These keep track of the debt and loans in the system and act as a coordination layer between other parties.

The system is designed to be extensible without the intervention of RCN. Anyone can model new kinds of loans, create their oracles, offer cosigning services, or act as loan approvers. This is achieved by interacting with user-provided contracts.

This level of flexibility makes an assessment of the security of the system as a whole virtually impossible. For example, an attacker could create a fraudulent loan-model contract and steal money by tricking people into using it. Similar attacks are possible with any user-provided contract.

Knowing this limitation, the main focus of this audit was on ensuring that the global integrity of the system is preserved and that any possible attack is self-contained. In other words, if an attacker manages to abuse the system, users who haven’t interacted with it shouldn’t be affected.

Audit Results

Medium severity issues

These vulnerabilities are considered of medium severity, as they don’t compromise the global integrity of the system, but are present in the core system contracts. Users interacting exclusively with well-behaved contracts are also affected by them.

[RCN1-M01] Some of LoanManager’s signed messages are too general

The LoanManager contract offers the possibility to separate the approval of certain actions from their on-chain execution, by accepting signed messages to approve them.

We found that the messages with the format “\x19Ethereum Signed Message:\n32” + _id have these problems:

1. They can be reused in multiple instances of the LoanManager.
2. They can be reused in unrelated contracts.
3. They have no expiration, and there’s no way of canceling a message for LoanManager#egisterApproveRequest.
4. They are interchangeable: the same message is signed for different actions.

We recommend changing the format of the signed message so that they include:

1. The address of the LoanManager contract.
2. A description or code of the intended action.
3. An expiration timestamp.

We also recommend implementing EIP-712.

Update: addressed in pull request #193.

[RCN1-M02] LoanManager#cancel can lead to request ids reuse

This function cancels loan requests by deleting an entry in the mapping of the requests. This can be abused in the following scenario:

1. User1 creates a loan request with params P. This request gets the id 1.
2. Its borrower approves it using a signed message (i.e. a signed personal message containing 1).
3. The borrower changes their mind and cancels the loan request.
4. An attacker recreates the loan request with params P, getting again the id 1.
5. The attacker can re-approve the proposal without the borrower’s intervention, forcing them to assume the debt.

We recommend adding a canceled flag to Request and implement the cancelation logic by setting it to true.

Update: addressed in pull request #194.

Low severity issues

These vulnerabilities are considered of low severity, as they don’t compromise the global integrity of the system, nor are present in the core system contracts.

[RCN1-L01] RateOracle#readSample can return inconsistent values

Calling this function can mutate the oracle’s state. This can be used to return inconsistent values for the same _data in a single transaction. If a smart contract uses it more than once inside a transaction, the oracle could abuse this to its advantage.

We recommend making it a view function. This way staticcall will be used when interacting with it, preventing this kind of attack.

Note that this attack would still be possible in a scenario including multiple transactions, or if another contract (e.g. the loan model) mutates the oracle state.

[RCN1-L02] Calling untrusted contracts without static calls should have strict gas bounds

When doing a non-static call to an user-provided contract, a strict upper bound of gas should be used. Otherwise, an attacker could have the user pay the gas of unrelated actions.

See Malicious GasToken Minting for an example attack.

[RCN1-L03] Possible reentrancy attack in LoanManager#settleLend

This function calls _validateSettleSignatures, which calls the borrower and the creator if any of them are contracts. Any of those contracts could call settleLend again with the same parameters. This can lead to the message sender transferring the borrowed tokens multiple times.

For this attack to be successful, calling LoanManager#_createDebt multiple times with the same values shouldn’t fail. And the same must happen with DebtEngine#create2, and Model#create.

While Model#create’s documentation mentions that it should revert if the loan already exists, we believe this could be abused by creating seemingly well-behaved model contracts. For example, implementing an idempotent Model#create wouldn’t look suspicious but would open the doors to this attack.

See [RCN1-R01] for recommendations.

Update: addressed in pull request #195.

Major recommendations

[RCN1-R01] Consider adding explicit reentrancy guards to every DebtEngine and LoanManager entry point

This system interacts with multiple user-provided contracts and has a large number of entry points, which causes a combinatorial explosion in the amount of possible reentrancy attack scenarios.

We strongly recommend adding explicit reentrancy guards to every public and external function of these contracts that modifies the state.

[RCN1-R02] Change the cosigning protocol

The current cosigning mechanism depends on reentrant calls to LoanManager. As explained in [RCN1-R01], we recommend preventing every reentrant call.

We believe that this logic could be refactored, moving most of the LoanManager#cosign code into LoanManager#lend.

[RCN1-R03] Duplicating logic should be avoided

We found multiple instances of duplicated logic. While we didn’t find any error related to this, we strongly recommend avoiding this practice, as it’s a common source of bugs and vulnerabilities.

We found that most of the cases had small variations in the logic (e.g. emitting different events, single vs batched execution). Our recommendation is to implement a single internal function per action, generic enough to support all the use cases.

[RCN1-R04] Use checked arithmetic or document the uses of its unchecked version

We found multiple places where unchecked arithmetic is used. We recommend switching to checked arithmetic or add comments explaining why each operation is safe.

Other comments and recommendations

[RCN1-O01] ByteUtils#read can fail silently

If a _length over 32 is provided, this function won’t act as expected, nor throw any error. We recommend validating this parameter.

[RCN1-O02] ByteUtils#decode functions don’t check their length parameters

If a length parameter over 32 is provided, these functions won’t act as expected, nor throw any error. We recommend validating these parameters.

[RCN1-O03] Out of bound reads in ByteUtils#decode with more than one length parameter

The versions of this function that receive more than one parameter have an error in the way they prevent out-of-bound reads. They don’t take the last length into account, making the require call miss some cases.

[RCN1-O04] Calling RateOracle#readSample always leads to duplicated logic

We found that every time this function is called, the same logic is repeated. We recommend changing its interface to avoid this. One possible solution is to add the functions tokensToCurrency(data, tokens) and currencyToTokens(data, currency).

Note that if this suggestion is implemented, [RCN1-L01] must be addressed too.

[RCN1-O05] ERC721Base#allTokens will eventually result in out of gas errors

This function returns an ever-increasing amount of data. Eventually, this data would be too costly to be used on-chain.

Update: addressed in pull request #185.

[RCN1-O06] ERC721Base#assetsOf may result in out of gas errors

This function returns an arbitrary amount of data controlled by the user. This could be abused, making this data be too costly to be retrieved on-chain.

Update: addressed in pull request #191.

[RCN1-O07] Some ERC721Base functions visibility could be restricted

We recommend making these functions private:

• ERC721Base#_clearApproval
• ERC721Base#_transferAsset
• ERC721Base#_addAssetTo

Update: addressed in pull request #186.

[RCN1-O08] Some DebtEngine#_safeGasCall #_safeGasStaticCall can result in out of gas errors

These functions try to impose a limit on the amount of gas an external call can spend.

When consulted with their developers, we were informed that their purpose is to let the DebtEngine contract store update a status variable if that happens. This isn’t always possible, as they sometimes use all the gas left.

Update: addressed in pull request #198.

[RCN1-O09] DebtEngine#_safePay doesn’t inform the caller that it failed

Calling this function always returns a valid value, even when the call to the model fails. This doesn’t lead to any errors at the moment, but that could happen if the contract is modified.

We recommend adding a boolean flag to its return value.

[RCN1-O10] Missing parameter validation in DebtEngine#_toToken

This function should validate that _equivalent is a positive number.

Update: addressed in pull request #187.

[RCN1-O11] Missing parameter validation in DebtEngine#_fromToken

This function should validate that _tokens is a positive number.

Update: addressed in pull request #188.

[RCN1-O12] Incorrect revert reason in InstallmentsModel#_validate

A require call checks that this condition is true: _timeUnit <= _installmentDuration. Its message indicates that it should never be true.

Update: addressed in pull request #185.

[RCN1-O13] Possible precision loss in InstallmentsModel#_newInterest

This expression (100000 * (_delta / _timeUnit) * runningDebt) / (_interestRate / _timeUnit) can be replaced with 100000 * _delta * runningDebt / _interestRate. This has a single division at the end, possibly losing less precision.

Note that _timeUnit gets canceled, so it doesn’t appear in the second version. This could be concealing a bug in the code.

[RCN1-O14] Consider merging InstallmentsModel#_decodeData and #_validate

These functions are used in conjunction. Consider merging them into one.

Update: addressed in pull request #196.

[RCN1-O15] Most InstallmentsModel’s function don’t validate ids

Many functions in this contract receive an id that points to a debt model. Most of these functions don’t validate that the debt actually exists. We recommend adding explicit validations.

Update: addressed in pull request #189.

[RCN1-O16] LoanManager#getDirectory will eventually result in out of gas errors

This function returns an ever-increasing amount of data. Eventually, this data would be too costly to be used on-chain.

[RCN1-O17] Some inline assembly blocks can be implemented in Solidity

These functions can be implemented in Solidity, making use of the abi.decode function:

• ERC721Base#_noThrowCall
• DebtEngine#_safeGasStaticCall
• DebtEngine#_safeGasCall
• LoanManager#_safeCall

Update: addressed in these pull requests.

[RCN1-O18] Many InstallmentsModel’s functions and edge cases are untested

We recommend implementing this contract’s tests in the same fashion that DebtEngine’s and LoanManager’s once, which are very thoroughly tested.

There are mentions to internal documents that contain testing data. We recommend inlining that data as comments.

[RCN1-O19] Helper.amostEqual could be concealing bugs

The InstallmentsModel’s tests use this function to compare some time-based results. We believe that these kinds of approximate comparisons when working with integer values can conceal bugs and we recommend against it.

Get a high-quality smart contract audit from Nomic Labs.