Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/CollateralAudit.md

Last active October 31, 2020 17:59
Show Gist options
  • Save nachomazzara/c8e1c5452a040d0fca28dc978dfd7ccb to your computer and use it in GitHub Desktop.
Save nachomazzara/c8e1c5452a040d0fca28dc978dfd7ccb to your computer and use it in GitHub Desktop.
Download ZIP
RCN - Collateral - Security Review
Raw
CollateralAudit.md

Collateral - Security Audit

Table of contest

Introduction

The RCN Team requested the review of the project on the repository https://github.com/ripio/rcn-network, the commit referenced for this audit is ddb339f49db7de0310753458982b7fbb02f95f19 (https://github.com/ripio/rcn-network/commit/ddb339f49db7de0310753458982b7fbb02f95f19). The contracts are under the branch collateral/master : The audited contracts are:

  • CollateralAuctionCallback.sol: Interface contract used by the Collateral contract where it is defined as the callback method to be called by the auction contract.

  • CollateralHandler.sol: Interface contract used by the Collateral contract where it is defined as the method that the Collateral contract will call to make a debt payment using collateral.

  • CollateralDebtPayer.sol: Contract used to pay the debt using collateral.

  • Colleteral.sol: Core contract where the collaterals are created and managed.

  • CollateralAuction.sol: Contract used to create the auction and execute the auctions based on its two pre-defined cycles.

  • CollateralLib.sol: Implements schemes and rules for a generic collateralization, and liquidations for under-collateralized entries

  • Fixed223x32.sol: A library that allows the usage of decimals.

The contracts are well written, some of them lack developer notation.

Also, the specification was audited too: https://github.com/ripio/rcn-network/tree/collateral/specification

The rest of the contracts in the repository are assumed to be audited.

General Notes

  • The command to run the tests is not working:

    npm run test
sh: bin/test.sh: No such file or directory

Specifications

Notes

  • N1 - [...] The exact conditions of the loan and collateral are determined only by the lender and creator of collateral. It is not clear who is responsible or accepts each piece.

  • N2 - [...] The amount of collateral offered for the constant amount of baseToken. This is valid in the first cycle. In the second one, it will be reduced by 0.

  • N3 - [...] Only one auction per collateral entry can exist at the same time, in the case of both liquidation conditions being met at the same time, the overdue liquidation has priority. In the code, claimLiquidation has priority. If this text is ok then there is a low vulnerability there.

  • N4 - Defines the rules and schemes of the collateral entries, abstracting the definitions of ratio, balance, canWithdraw, and inLiquidation; for better readability and testing purposes. Collateral.sol makes internal use of this library. toBase is missing here.

  • N5 - The consignment requires that the collateral/debt ratio to be above the defined _liquidationRatio, and this is intended to impede the owner of the collateral to front-run the lend transaction with a withdraw transaction.. In the code, if the collateral/debt ratio is equal to the _liquidationRatio, the loan can be consigned.

interfaces/CollateralAuctionCallback.sol

Notes

  • N1 - Missing dev notation. Please consider adding notation explaining what the contract does.

interfaces/CollateralHandler.sol

Notes

  • N1 - Missing dev notation. Please consider adding notation explaining what the contract does.

utils/CollateralDebtPayer.sol

High Severity

  • H1 - line 107 - Lack of support for tokens without a return value. To be compatible with CollateralDebtPayer.sol contract, an allowed token should return true upon a successful transaction. this is not met in some implementations, like OMG (OmniseGO), BNB (Binance coin) and other not fully ERC20 compliant tokens.

For collateral, the RCN team tell us that the base token is always the RCN token which is fully ERC20 compliant and therefore this issue does not apply.

Medium Severity

  • M1 - Line 81 - Possible underflow. If the amount passed on the _data parameter is greater than _total, a possible underflow could happen when calculating the surplus in line 113. This also ends up on an approving of a higher value than the expected. In order to avoid this, consider using the SafeMath.sol library or check for an underflow.

This issue seems to not be reachable by using it within the Collateral contract because both parameters are checked before calling the handle method.

Low severity

  • L1 - line 107 - Possible loss of funds. The extra base token is being sent to the owner of the collateral where this owner could a contract without the possibility to move the tokens. Consider adding to the _data (action) a beneficiary or _to address where the extra funds can be transferred. Another solution could be to pass the tokens back to the Collateral address and decide there what to do.

Notes

  • N1 - Line 44 - Function encode seems to not be used.

The RCN team will leave this method in case anyone wants to use it for better readability.

  • N2 - lines 139 and 164 - Lack of check if an approval was a success. Related to H1, some tokens do not revert if the approval was not a success. Consider using safeApprove + require to fully check that the approval was successful.

  • N3 - line 29 - Generic Name - The data field for an Action is used to store the Oracle data, also data is a generic name that can refer to multiple things. Consider changing its name to oracleData for readability purposes.

Collateral.sol

Low Vulnerabilities

  • L1 - Line 291 - Possible loss of funds. The collateral contract is not fully decentralized because it has an owner. Using the onlyOwner modifier to extract the collateral in case of the loan fails will cause a lock of the funds and therefore impossibility to move them if the owner is vulnerated or lost. Consider allowing an authorized address to also redeem the collateral.

  • L2 - Line 326 - Limited flash loans. borrowCollateral set the flag which is preventing for re-entrancy and therefore the benefits of a flash loan can not be used. A use case could be to deposit more collateral and/or withdraw from another collateral.

  • L3 - line 637 - Wrong overdue check. A collateral can be claimed if the time passed is equal to the due date. This means that If someone has a system that tries to pay in the last second, someone can claim the collateral in the same block by front-running it.

  • L4 - line 649 - Possible estate mutation - A non-static call is being made when trying to get the market value for the obligation tokens by reading the collateral oracle. This call could end up in a possible state mutation. Consider changing read to readStatic.

Notes

  • N1 - line 122 - Missing constructor's dev notation.

  • N2 - Line 114 - Possible incorret setup. Missing checks for _loanManager and _auction whether they are contract or not. Also, _loanManager should return an address related to a token. This address is not being checked either.

  • N3 - line 130 - Forced owner. The owner of the collateral will be the msg.sender. Sometimes users create their own systems on top of others and a contract with the impossibility to move funds and/or the collateral token could be the sender of the collateral creation. Consider adding a beneficiary parameter who acts as the owner of the collateral token.

  • N4 - Lines 169 and 219 - Garbage storage and events. A collateral can be created with amount 0. Also, a deposit of 0 is possible. Consider adding a check to prevent human mistakes and/or malicious/obsolete logs as Created, Withdraw, Deposit.

  • N5 - Line 169 - Obsolete collateral creation. A collateral can be created with a liquidation ratio lower than the current ratio. Consider checking the current ratio in order to avoid obsolete collateral creations.

  • N6 - Lines 171 and 299 - Usage of arbitrary values. Status 0 represents an open loan and 4 represents error. Those values are arbitriary and non-semantic. Consider using an enum or a constant for loan statuses to make the code more readable.

  • N7 - line 265 - Typo. withdew to withdrawn.

  • N8 - line 326 - Prevent excessive gas consumption. If someone tries to pay a loan which is totally paid or borrow from a collateral which is being auctioned, a lot of contract calls and checks will happen to end in a possible revert or an obsolete call and therefore a waste of money in gas fees that can be totally prevented. Consider checking if the debt has been paid and/or the collateral is being auctioned before start calling the other contracts to save gas.

  • N9 - line 355 - What status 2 means? Consider using an enum for status or at least constants.

  • N10 - line 498 & 517 - Variable loaded twice from storage. loanManager is being loaded from storage twice. Consider using a variable in memory to reduce gas consumption.

  • N11 - line 543 - Wrong visibility. claim is public. Consider changing it to external.

  • N12 - line 543 - Prevent excessive gas consumption. If an auction is in progress, the claim method can return false to avoid calls and further checks. Consider checking if the collateral is being auctioned.

_ The RCN team has been asked what happen also if the loan has an error (status = 4). They haven't had the path in mind, but all agree that checking for the status and if the collateral is being auctioned could be the right solution._

  • N13 - line 582 - Not all path returns a value. At _claimLiquidation, if the collateral is not in liquidation, the method will return false by the default value of an uninitialized bool but is not clear at first sight. Consider adding a return false if the collateral is not in liquidation.

  • N14 - lines 604, 668 - Lack of event standardization. ClaimedLiquidation and ClaimedExpired are events that happen if an auction will be triggered. They have mostly the same arguments but with different names and also in different positions. Consider using the most important and equal arguments first.

  • N15 - line 703 - Possible code repetetion. _debtInTokens is similar to toToken. Consider refactoring them using a new param: _ceil in favor of code reutilization.

  • N16 - line 732 - There is a TODO in the code.

  • N17 - lines 642 and 739 - Impossibility to claim - If the debt is expired and the obligation is higher enough that when the mult(105) happens, an overflow occurs, a debt could be impossible to be claimed. The same happens on line 739 if _marketValue is higher enough. Consider checking first if the obligation will overflow, and return the max uint256 possible.

If that value is not possible to be achieved, this note hasn't effect.

  • N18 - line 306 - Wrong comment. // Destroy ERC721 collateral token is wrong. It is only freeing the storage and clear al the fields from the Collateral struct.

  • N19 - line 216 - Obsolete transfer of funds. Anyone can deposit more collateral for a valid collateral entry even if the debt has been paid and/or has an error. Then, an authorized account should withdraw those funds to recover them. Consider doing debt's status checks before allowing a deposit.

  • N20 - line 503 - Possible error misunderstood. If someone is trying to cosign a loan with an invalid/unexisting collateral an error saying collateral: incorrect debtId will be throw but, maybe the correct error is that the collateral is invalid. Consider checking if the collateral is valid before checking the incorrect debtId error.

  • N21 - line 412 - Unexpected amount transferred. When an auction is closed, the requested amount will be used to pay the loan. If the load has an error, safePayToken will return 0 as the amount of token paid and therefore all the requested amount is going to be transferred to the owner of the collateral. Related to this, when collateral is being auctioned and the loan has an error, no collateral amount could be redeemed because it will be 0. Consider adding it to the documentation if this is the expected behavior or a mechanism to fix it manually by the owner of the Collateral contract.

To take into consideration

  • The auction contract is not updateable. If for some reason the auction contract has an issue, all the collaterals created won't be able to be auctioned and therefore a new collateral contract should be created along with a different auction. Maybe adding a method onlyOwner to update the auction contract can be helpful.

  • Inconsistency between the delete of the collateral data and the ERC721 token. Collateral is a non-burnable ERC721 but its data is cleared once it is redeemed. The total supply of the contract is public and the method getEntriesLength allows to iterate through each collateral where many of them could have its value cleared and therefore its info could not be retrieved on-chain.

CollateralAuction.sol

Notes

  • N1 - line 182 - Lack of information. take function is being called without knowing the total amount that will be debit from the sender and therefore is going to be calculated there. Then, onTake function, from the sender, is being called without any parameter. The only information avoiding any extra call the sender has at that moment is the auction id but not the number of tokens debited. This information can be really useful to log or store what happened. Consider adding a data parameter to onTake with rich information as the amount debited.

  • N2 - line 260 - Typo. sould to sold

  • N3 - line 272 - Unscoped variable initialization. The deltaAmount variable is only neccesary if deltaTime < _auction.limitDelta. Consider moving the variable initialization to line 276.

  • N4 - line 303 - Inconsistency between dev notation and code. Depending on the values, this operation could never achieve 1 as the dev notation says. Consider changing the dev notation if reaching to 1 is not mandatory.

CollateralLib.sol

Low severity

  • L1 - line 61 - Wrong require condition. The _liquidationRatio should be above 1, but the condition is inclusive >= (greater or equal to). Consider changing it to > (greater to).

  • L2 - line 192 - Possible loss of funds. At canWithdraw a conversion to baseToken and to toToken are being made. If the debt is 0, the same amount is being converted from token (N) to baseToken (X) and baseToken (X) to token (M) where some oracles can return different values (N != M ). If that happens, an small amount of tokens can be locked forever in the contract. Consider returning the collateral amount if the debt has been paid, its value is 0.

Notes

  • N1 - lines 130 & 156 - Repeated contract call. sample.toTokens(_col.amount) is being called twice with the same parameters and without calling any extra method which can modify the rate. Consider saving the value in memory to reduce gas consumption.

  • N2 - line 187 - Exclusive condition. If base is equal to limit, the operation at line 191 will call the oracle contract and therefore returns 0. Consider checking for equal too to reduce gas.

  • N3 - line 138 - Exclusive condition. If limit is equal to base should returns 0. Consider checking for equal too to reduce further operations' gas consumption.

Fixed223x32.sol

High severity

  • H1 - The library uses 224 bits (28 bytes) for the number and 32 bites (4 bytes) for the fraction. This means that the max number that this library can handle is 2 ^ 224 instead of 2 ^ 256. After that number, the library should overflow. Functions as from reverts if a number bigger than 2 ^ 224 is passed, the same happens to div. Functions in CollateralLib.sol as canWithdraw could revert if the debt is greater than that value too.

    • For the lib, consider using a max number or split the number and the fraction into two different variables by using a struct.

    • For its usage in Collateral, if a debt and/or collateral amount in base token could be higher than 2 ^ 244 consider removing it if no changes to support higher numbers will be applied to the lib.

Notes

  • N1 - Multiple lines - Manual casting. The function raw is casting a uint256 to bytes32 and in almost every function, a manual cast to bytes32 is done. Consider replacing the cast by calling raw.

To take into consideration

  • Sometimes checking for inclusive ranges is useful. Consider adding gte and lte functions to the library.

While the audit, the RCN team realized that the library should be named Fixed224x32 which is the proper name.

Ignacio Mazzara - 03/2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment