search

Consensys / defi-score

DeFi Score: An open framework for evaluating DeFi protocols
https://defiscore.io
Other
280 stars 80 forks source link

Updated Smart Contract Security Guidelines #39

Closed jclancy93 closed 4 years ago

jclancy93 commented 4 years ago

Overview

This proposal outlines a new, more robust set of audit requirements for DeFi Score.

Problem Statement

The original version of DeFi Score used a simple boolean value on whether or not a DeFi protocol’s smart contracts were audited or not. A boolean value does not capture the complexity that is inherent in smart contract security auditing. All audits are not created equal. There are many facets of a smart contract audit that help determine its quality and the security of the underlying protocol.

In light of recent events in the DeFi ecosystem, we have decided to propose a more robust framework for evaluating smart contract audits and the underlying protocols level of security. We hope that these guidelines will better capture the full spectrum of how different DeFi products approach security.

There is no silver bullet for smart contract security. Audits performed over many engineering weeks by security firms with sterling reputations still miss critical and high severity issues. That being said, a rigorous approach to security helps minimize the probability of such events.

Proposal

Protocol’s security:

Teams like Compound have been extremely diligent about continually auditing their protocol’s code as they introduce new functionality and code modules, while other teams have taken a more lax approach, with the last audit happening a year or 18 months ago. The guidelines below hopefully better capture the different levels of rigor better than the previous iteration of our audit guidelines

The proposed requirements for receiving audit credit are now:

A simple score is never going to capture the complexity of security auditing and is not necessarily meant to. Abstractions are always leaky. Most people, even in the DeFi ecosystem, do not have the capabilities/skill set to independently verify the security of smart contract protocols. It is our hope that these guidelines help set a minimum standard for team’s approach to security and allows for laypeople to make better decisions about how safe it is to deposit their money in different protocols.

Alternatives

Have a centrally managed list of supported audit firms This is a concept we’ve discussed in the past, but it seems hard to manage and might be seen as biased since there may be conflicts of interest with Diligence Using some time decay function that lowers the security score of a protocol based on time since the last audit Something like this could definitely be useful, but it seems rather reductive because an audit may only cover a new module, which does not provide any guarantees on other modules that may have changed. This is also contrary to the idea that being onchain without being hacked for more time is a good proxy of security. (assuming no upgrades).

ie. Uniswap shouldn't have to reaudit the same code every year.

Open Questions

  1. Do we have any recommendations/guidelines for economic audits?
  2. How do we keep track of vulnerabilities and audit results?
  3. How do we give credit to teams that may have a more internal security function
sneg55 commented 4 years ago

can you define "minimal code changes" ?

maurelian commented 4 years ago

Can you confirm that these guidelines are the ones we should be reviewing rather than the ones in the blog post?

jclancy93 commented 4 years ago

can you define "minimal code changes" ?

Still tbd. Looking for community input on this. It may be that minimal = 0. We wanted to add this caveat because protocols like Uniswap should not need to be audited every year if they haven't made any changes

Can you confirm that these guidelines are the ones we should be reviewing rather than the ones in the blog post?

Hey John, yea that's correct. Good to have you here 🙂

tlip commented 4 years ago

• Has Sam Sun looked at it (jk but not really)

lol

tlip commented 4 years ago

Overall, I like this improvement to the methodology. I think that this brings some much needed depth to the Audit factor of the DFS.

A few thoughts. All of my questions are more-so "what if?" hypotheticals in order to account for situations that may arise in the future...

• An audit has taken place in the last 12 months OR there have been minimal code changes since the last audit (15%)

Even if a protocol's contracts themselves haven't been updated since before their last audit but there have been updates to the EVM? Although I know this risk is low (in a perfect world, this risk wouldn't exist), I don't think it's exactly non-zero.

Does "code changes that address the results of the previous audit" fall into the "minimal code changes since last audit" category?

• Audit results must be released publicly (15%)

What happens if a protocol has 3 audits while only the results for 2 of them are released? Corollary questions:

In the end, good job, @jclancy93 !

jclancy93 commented 4 years ago

Even if a protocol's contracts themselves haven't been updated since before their last audit but there have been updates to the EVM? Although I know this risk is low (in a perfect world, this risk wouldn't exist), I don't think it's exactly non-zero.

This is a good question and isn't something I've thought about before. Aragon faced an issue like this recently. The main risk here isn't that contracts will become more exploitable due to EVM upgrades, but will instead be bricked. You're definitely right that the risk is non-zero, but I think EVM upgrades that potentially break contracts might be out of scope for this methodology as long as they don't compromise the security of a contract.

Does "code changes that address the results of the previous audit" fall into the "minimal code changes since last audit" category?

Yes, they do. They are typically reviewed by the auditors to make sure that they properly address the disclosed issues.

What happens if a protocol has 3 audits while only the results for 2 of them are released? Corollary questions:

  • What if a protocol's first audit is private but its subsequent audits are made public?
  • What if there's a private "general audit" for the entire protocol, but a public audit for a new feature (or vice-versa)?
  • Other similar questions for various scenarios could probably be thought up

As long as one audit is public, protocols will receive credit.

sneg55 commented 4 years ago

might be out of scope for this methodology as long as they don't compromise the security of a contract.

I disagree with this, assessing risk in permissionless lending platforms it's not equal to smart contract security only.

jclancy93 commented 4 years ago

I disagree with this, assessing risk in permissionless lending platforms it's not equal to smart contract security only

Agreed. That's why we try to take a holistic view of risk by looking at smart contract risk, financial risk, and centralization risk. Also, this score considers systemic risk like Ethereum's consensus mechanism breaking or a 51% attack out of scope. Breaking EVM changes seems like another systemic risk.

Without more examples of EVM changes breaking a contract's security guarantees, we don't really have much information to operate on.

The Aragon example is the only notable example of this happening I can remember. If breaking changes like this become more common, I would expect auditors to also pay attention to experimental or soon to be deprecated solidity/vyper features to help their clients avoid this.

If anyone from the DeFi Score community has a proposal for trying to assess the risk that future EVM versions break a smart contract, that would be a very welcome contribution.

maurelian commented 4 years ago

Yeah, auditors should be advising clients about EVM upgrade risks. The only risk I'm currently aware of is related to making safety assumptions based on gas prices remaining constant (as with the case of Aragon discussed above).