RCN - Fee & burner - Security Review

RCN_Fee_Burner_Security_Review.md

Fee & Burner - Security Review

Table of contest

• Introduction
• Contracts
  • DebtEngine.sol
  • BurnerConverter.sol

Introduction

The RCN Team requested the review of the contract DebtEngine.sol (https://github.com/ripio/rcn-network/blob/a3cb62323e36cc751e96150926c6fd2b288628ea/contracts/core/diaspore/DebtEngine.sol) referenced by the commit https://github.com/ripio/rcn-network/commit/a3cb62323e36cc751e96150926c6fd2b288628ea. And, the contract BurnerConverter.sol (https://github.com/ripio/rcn-burner/blob/5bb3ffc0d998a78bba0080656af06a7ea0945dc4/contracts/BurnerConverter.sol) referenced by the commit ttps://github.com/ripio/rcn-burner/commit/5bb3ffc0d998a78bba0080656af06a7ea0945dc4.

The contracts are well written, many functions lack radspec.

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

Contracts

DebtEngine.sol

Low severity

• LS1 - line 762- Possible total payment amount mismatch - At getFeeAmount the fee value used to get the amount is the current fee and not the debt fee. Consider using different functions to get the amount in the current fee if it is for the creation process and another one to display the amount for paying an already created debt.

Notes

• N1 - lines 127 and 134 - At setBurner and setFee, if the owner tries to set the already set values, it will compute and emit an obsolete event. Consider checking if the value about to be set is different from the current one.

• N2 - line 531 - When the burn amount is being calculated, many WEIs can be "lost". Consider if it worth checking if the result of burnAmount = _amount.multdiv(_fee, BASE) is 0 rounding it up to 1 or prevent wasting gas by pulling 0 from the payer and emitting the ChargeBurnFee event.

• N3 - lines 669, 680, and 704 - Consider checking if the amount and/or balance of each debt is different from 0 to avoid obsolete events and excessive gas consumption.

• N4 - line 684 - The require(debt.balance >= _amount, "Debt balance is not enough") is not necessary because of the usage of safeMath lib in the following line (685). There is the same check at sub require(x >= y, "Sub overflow"). Also, there is a typo at enought.

• N5 - line 705 - Lack of usage of SafeMath. Even the probability of experiencing an overflow is low, if for some reason total ends up being higher than the highest number possible, the beneficiary will lose funds.

To take into consideration

• C1 - lines 144, 149, and 217 - At each create functions, the id of the debt is generated by encoding different parameters. The same is done by the build functions. Consider using the build functions in order to reduce the risk of having duplicated code with errors.

• C2 - Lines 309, 368, and 509 (423 and 458) - If _safePaid finished with error, a lot of computation can be omitted by returning immediately because paid will be 0. Also, a Paid event is emitted even if an error happened at the module level

• C3 - lines 329, 387, and 522 - The value 340282366920938463463374607431768211456 is triplicated through the contract to check if the balance has suffered an overflow. Consider moving it to a constant to reduce human errors by copying and pasting it.

• C4 - lines 720 and 729 - When the debt is in error, the value 4 is set. This value is arbitrary and non-semantic making it hard to read. Consider using a constant.

BurnerConverter.sol

Notes

• N1 - line 36 - The direct burn of _soldAmount can be performed if _soldToken is equal to burnToken, reducing logic and excessive gas consumption.

To take into consideration

• C1 - line 15 - The converter contract can't be changed. If for some reason, the current converter contract gets old or has a bug, the BurnerConverter contract should be deployed again and also be changed on every place it is being used. Another alternative is to pass the converted at the function executeBurning, which is only allowed to be called by the owner.

• C2 - line 36 - In order to reduce gas, the transfer of the tokens to the converter can be done instead of approving them first. If the contract has no balance the tx will revert immediately.

• C3 - line 45 - At executeBurning the output of the convertFrom function is used to check against the minReturn expected. There are a lot of deflationary tokens around and if the converter is not returning the balance of the BurnerContract after the transfer (and the require passed), the burn will fail because the balance will mismatch.

• C4 - line 45 - Lack of support for tokens without accepting the address 0x as the recipient of the amount for transfers. Some tokens like BNB (Binance coin) don't support transferring tokens to the address(0) https://etherscan.io/address/0xB8c77482e45F1F44dE1745F52C74426C631bDD52#code (line 82)

Ignacio Mazzara - 09/2020