We at DeFiner give high priority to the security of our Smart Contracts. We make sure that community funds in our Smart Contracts are safe from different sorts of security attacks. DeFiner recently hired ConsenSys Diligence to perform the security audit of our Smart Contracts. You can read the published report here.
In the security audit report from ConsenSys Diligence, they found 1 critical, 2 major, and 5 medium issues. The good news is we have fixed all critical and major issues, and fixed some medium issues, too.
We also submitted our fixes and their respective Pull Requests on GitHub to the ConsenSys Diligence team to review and update their report. However, due to the immediate unavailability of the resources on the ConsenSys side, the report only lists found issues. Therefore, we decided to cover the fixes in this blog post.
The issues in the audit report are as follows (you can see DeFiner’s comment on each issue below):
4.1 Users can withdraw their funds immediately when they are over-leveraged [Critical] [Fixed-awaiting-review]
DeFiner’s comment: In the `withdraw` function, we disallow withdrawals if the user is already over-leveraged. For liquidation, we wrote an additional withdrawal method to support liquidation, and the function is named `withdraw_liquidate`.
There are two key differences between `withdraw` and `withdraw_liquidate` functions in `Account.sol`. First, in `withdraw_liquidate`, an account is allowed to withdraw tokens (not directly, only from authorized contracts and in liquidation function) without checking if its collateral is enough to support its borrow position. Second, DeFiner doesn't take a commission in `withdraw_liquidate`.
We have tested these new changes after adding the `withdraw_liquidate` function.
4.2 Users can borrow funds, deposit them, then borrow more [Major] [Won’t Fix]
DeFiner’s comment: This is expected behavior and our smart contracts are designed like this. Other lending protocols like Compound and AAVE allow this feature as well. User’s funds are not at risk, because it’s an ‘expected feature.’ The funds of the users are only at risk when their position is over-leveraged, which is also an ‘expected behavior.’
4.3 Stale Oracle prices might affect the rates [Major] [Won’t Fix]
DeFiner’s comment: We will work on this issue in our upcoming release. Also, we will add some back-up oracle plans. And because of this, we won’t fix this issue in this version.
4.4 Overcomplicated unit conversions [Medium] [Fixed-awaiting-review]
DeFiner’s comment: There is a tradeoff on using a wrapper function. The wrapper function could introduce more multiplication and division operations on the UNIT variable (10**18). In our current implementation, the UNIT multiplication and division could be removed somewhere as a common factor and save on gas costs.
4.5 Commented out code in the codebase [Medium] [Fixed-awaiting-review]
DeFiner’s comment: We have reviewed the code and removed unnecessary comments.
4.6 Price volatility may compromise system integrity [Medium] [Won’t Fix]
DeFiner’s comment: This issue says that due to price volatility there could be an attack on DeFiner. However, price volatility is inherent in the Cryptocurrency ecosystem. All the other lending platforms like MakerDAO, Compound, and AAVE are also designed like this, in the case of price volatility (downside), more liquidation happens on these platforms as well.
Liquidations are good in a sense because they keep the market stable. If there is no liquidation during a market crash, the system will be at risk. Due to this, it is always recommended to maintain the collateral and borrow ratio by the user. A user should keep checking their risk position on DeFiner’s website if the market were to possibly crash. It’s always a good idea to keep an eye on the health of your investments just in case the market were to tumble towards the downside.
4.7 Emergency withdrawal code present [Medium] [Fixed-awaiting-review]
DeFiner’s comment: Emergency withdrawal code was removed from the contracts. This was added to safeguard the user’s funds just in case of an attack on our smart contracts.
4.8 Accounts contains expensive looping [Medium] [Fixed-awaiting-review]
DeFiner’s comment: In the current design of the contract, we have to iterate on top of all supported tokens in calculating borrow power and other variables. Therefore, the loops are necessary, and we aim to reduce the complexity inside a loop to optimize gas costs.
We noticed that there are repeated cross-contract calls inside the loops, which is expensive. We could just do the cross-contract call once and save the result in a local variable for reuse. Based on this observation, we made two improvements.
4.9 Naming inconsistency [Minor] [Won’t Fix]
DeFiner’s comment: Our contracts are tested after integrating with the UI after many tests. If we change the name of the function, we would have to make a lot of changes. At this point, we don’t want to change a lot of files. And because of this, we won’t rename the function name, because this is not causing any security issues.
Conclusion:
The ConsenSys Diligence found 1 Critical, 2 Major, and 5 Medium severity issues in our smart contracts. The “Critical” and the “Medium” severity issues have been fixed and sent for another round of review to our auditors. Both of the “Major” severity issues are indeed not the actual issues, and because of this, we will not fix these. We are hoping to get a review from the auditor soon for the fixes we’ve already made to our smart contracts.