Making Cover SAFU — Secureum #5
Security Audits For Users
Previous two posts (#3, #4) hypothesized that we can take a step towards making DeFi “SAFU” by breaking down Security Audits For Users (SAFU) and illustrated that with case studies of DeFi stablecoin and Zk-Rollup payment projects.
This post dives into another interesting DeFi project — Cover Protocol, a peer-to-peer coverage market for allowing DeFi users to insure against smart contract risk.
To provide some context for new readers (others may skip this section), DeFi projects rely on external security audits as a stamp of security approval. Audit is not a security warranty of “bug-free” code by any stretch of imagination but a best-effort endeavour by trained security experts operating within reasonable constraints of time, understanding, expertise and of course, decidability.
Audit reports illustrate security issues with descriptions of underlying vulnerabilities, likelihood/impact, potential exploit scenarios and recommended/resolved fixes. They also provide subjective insights into code quality, documentation and testing. The scope/depth/format of audit reports varies across auditing teams but they generally cover similar aspects.
Security companies execute audits for clients who pay for their services. Engagements are therefore geared towards priorities of project owners and not project users. Audits are not intended to alert potential project users of any inherent risk. That is not their business/technical goal.
Nevertheless, the conjecture is that project users may be basing their risk without considering if the project had any security audits or incorrectly assuming no/minimal risk for audited projects without bothering to, or having the expertise (understandably so) to evaluate audit report contents.
Evaluating audit reports requires a reasonable level of expertise in smart contract security. Breaking down security audit reports for the benefit of less security-savvy users may make DeFi a bit safer by helping them DYOR.
This post gives a breakdown of the audit reports on Cover Protocol from PeckShield, Arcadia Group, MixBytes and Maxsam4, and draws attention to key report highlights, in the context of the exploit and incident response.
Context
Project Overview: Cover Protocol allows DeFi users to protect themselves against smart contract risk by buying insurance coverage against a particular protocol until a specific expiration date by depositing a premium amount in the preferred collateral (e.g. DAI).
From the documentation: For each DAI deposited, the user receives 2 tokens, a CLAIM token and a NOCLAIM token. The NOCLAIM token represents rights to receive the deposited collateral in the event that a claim payout is NOT awarded during the designated coverage period. The CLAIM token represents a right to receive the deposited collateral (or a fraction thereof) in the event that a claim payout is awarded by the claims management process.
There are three types of participants in the Cover Protocol market: Market Makers, Coverage Providers, and Coverage Seekers. Market makers hold both CLAIM and NOCLAIM tokens and provide liquidity in DEX pools. Coverage providers hold and provide liquidity for only NOCLAIM tokens. Coverage seekers hold only CLAIM tokens to cover the exposure to the protected product.
The claims managements process allows anyone to file a claim against a protected product by paying the claim file fee. Token holders vote to decide whether it is a valid or invalid claim. Valid claims will be passed to a Claims Validity Committee (CVC) which is made up of security auditors (currently includes PeckShield, Arcadia, Haechi, Hacken and others) and evaluates the claim against the exploit incident to make a final decision on claim validity and payout percentage.
Exploit Overview: On 28 December 2020, a bug was exploited in Cover Protocol’s shield mining contract Blacksmith.sol that allowed one to mint infinite $COVER tokens. This was exploited multiple times resulting in a loss of over $4M.
The Cover team responded by immediately removing minting rights from the exploited contract, performing a detailed post-mortem, announcing a compensation plan for those affected, relaunching the governance token, stopping shield mining and reaffirming security as their top priority by continued focus on extensive testing, internal/external code reviews and a minimum of two official security audits.
The technical underpinnings behind the exploit are best explained here and here. To summarize, the pool data which was cached in memory (to presumably save on gas) is invalidated when updating the corresponding data in storage. However, the invalidated memory-cached pool data gets incorrectly used thereafter, resulting in the vulnerability. This is illustrated in the code snippet below:
This vulnerability detection depends on data-flow analysis that is also specific to the contract logic and is unlikely to have been caught by automated static analysis tools. Manual analysis should however have detected this inconsistency.
Audits Overview: There have been four audits covering different components: first with PeckShield covering core protocol and claim management, second with Arcadia on shield mining (deprecated now) which included the exploited BlackSmith.sol contract, third with MixBytes (via Yearn Finance) on the intermediate routing layer and most recently by Maxsam4 on bonus token rewards.
The PeckShield audit was during November 2020 with 2 auditors reporting 0 critical, 1 high, 2 medium, 4 low and 1 informational severity issues. All issues were fixed.
The Arcadia audit reported on 1 December 2020 with 1 auditor finding 1 high, 2 medium and 1 low severity issues. All issues were fixed.
The MixBytes audit was a 3-week audit (25 December 2020 to 11 January 2021) with 2 auditors reporting 0 critical, 2 major, 4 warning and 1 comment severity issues. Except the warning on front-running, which was acknowledged, all other issues were fixed.
Maxsam4’s audit was reported on 18 January 2021 with the single auditor reporting 0 critical, 3 major, 3 minor, and 2 informational issues with 7 of them fixed and 1 acknowledged.
PeckShield Report Breakdown
The audit report is considerably elaborate.
Introduction: OWASP Risk Rating Methodology is used for evaluation which is a good standard to adopt instead of coming up with a new one. The High/Medium/Low ratings of Likelihood and Impact are combined to classify Severity into 4 categories as shown below:
Vulnerabilities of 36 types across 4 categories (details in report Appendix) and CWE sub-categories are investigated using a combination of proprietary static analysis tool, manual analysis, contract deployment on private testnet and analysis of business logic implementation vis-a-vis specification, as shown below:
Findings: Key aspects of Findings and their Status is summarized below. As noted, all issues were fixed subsequently.
PVE-001 calls out failure to check return values of transfer()/transferFrom(), which is a common error that has caused issues in the past. PVE-002 calls out reentrancy issue which is yet another common error that can be fixed by using the checks-effects-interactions pattern or using reentrancy guard as was done here. PVE-003 calls out the need for special handling of deflationary/rebasing tokens. PVE-004 is a missing sanity check on an input parameter which wastes gas in this case. PVE-005 suggests the use of increaseAllowance()/decreaseAllowance() to avoid the classic approve()/transferFrom() race condition. PVE-006 suggests the use of a two-step ownership transfer mechanism to avoid contract freezes due to typos in addresses while executing critical address changes. PVE-007 illustrates a high-severity front-running vulnerability in the contract logic. PVE-008 calls out an incorrect event emission.
The report concludes by noting: "current code base is clearly organized and those identified issues are promptly confirmed and fixed.”
Arcadia Report Breakdown
The audit scope included five smart contracts (BlackSmith.sol, COVER.sol, Migrator.sol, Vesting.sol and MerkleProof.sol) and all issues found were fixed and reviewed. A combination of static and dynamic analysis was used, with each approach finding 2 of the total 4 vulnerabilities as reported under Finding Type.
COVER-1 calls out a bug in the handling of non-bonus dust tokens. COVER-2 calls out a missing check for token balance while adding bonus tokens, which would make existing unclaimed bonus inaccessible. COVER-3 calls out a bonus token balance bug due to the use of a strict equality check. COVER-4 (erroneously numbered COVER-5 in report) refers to a missing check to account for deflationary token handling.
All 4 reported findings, including the high-severity COVER-2, are related to handling of bonus tokens in BlackSmith.sol, which was also the contract with the undetected vulnerability that was subsequently exploited.
MixBytes Report Breakdown
This audit was executed on behalf of Yearn Finance (with which Cover merged) to assess the intermediate layer between Cover Protocol core contracts and end-users.
Their Security Assessment Methodology starts with a “Blind” phase which involves inferring the project’s specification/architecture by only reviewing source code. This is followed by checking for well-known vulnerability categories and then evaluating business logic and project architecture by reviewing code, documentation and tests.
The audit focussed on contracts CoverRouter.sol and Rollover.sol (in the cover-peripheral repository, which appears to be private/inaccessible for now) along with some typical utility contracts.
MJR-1 finding is about vulnerability to flash loan attack from token liquidity manipulations. MJR-2 finding is about vulnerability to token loss because of missing validation checks and unlimited token approval. WRN-1 warning is about missing zero address checks in the constructor. WRN-2 warning is about vulnerability to front-running attacks from mempool visibility of transactions. WRN-3 warning is about missing checks on balance token transfers related to the business logic. WRN-4 warning is about an unlimited approval access. CMT-1 comment is about missing events for critical onlyOwner parameter changes. All recommendations were fixed except the one on front-running which really is an Ethereum-level concern being addressed by approaches such as Miner-Extractable-Value (MEV).
Maxsam4 Report Breakdown
This audit scope included the Cover Protocol bonus token rewards contracts and reported 8 issues (0 critical, 3 major, 3 minor, and 2 informational) with 7 of them fixed and 1 acknowledged. It states that: “Overall, the code follows high-quality software development standards and best practices.”
Major-1 highlights a potential DoS exploit which could prevent addition of new pools via front-running. Major-2 highlights a scenario that makes it impossible to claim rewards because of “==“ check instead of “<=“. Major-3 highlights the need for special treatment for collecting dust ETH. Minor-1 highlights the need to differentiate the handling of LP and bonus tokens. Minor-2 highlights the incorrect sharing of a data structure across pools. Minor-3 is about gas optimization. Informational-1 is a typo in the README. Informational-2 is a suggested performance optimization.
Conclusion
This post gave a breakdown of audit reports on Cover Protocol — a peer-to-peer coverage market for allowing DeFi users to insure against smart contract risk — from PeckShield, Arcadia, MixBytes and Maxsam4, in the context of the exploit which happened after the first two audits. Overall, there were some red flags raised by the audits, prior to the exploit, which were fixed. But the exploited vulnerability unfortunately went undetected in Arcadia’s audit.
The exploited vulnerability was not a common Solidity coding pitfall that could have been detected by automated tools checking for security best practices. It was a programmer error related to caching inconsistency involving EVM’s memory and storage, and was also particular to the contract’s logic. More careful manual analysis could probably have caught it.
Security hindsight, after an exploit, is certainly 20/20 and regretful. Better specification, more documentation, extensive testing of corner cases, use of security tools during development, internal code reviews, multiple independent audits covering the same scope for redundancy and a public bug bounty program would help raise the bar.
It is a bit ironic that a protocol supposed to insure users against smart contract risk was itself exploited. Vulnerabilities can be reduced but cannot be ruled out. Guarded launches with built-in circuit breakers and better on-chain monitoring are therefore critical.
Breaking down DeFi Security Audits For Users (SAFU) may be worthwhile as a public good but certainly should not be interpreted as an endorsement/indictment of projects/auditors or as financial/investment advice. DeFi SAFU should be a collective community responsibility.
I hope you found this somewhat useful. Thanks for reading and look forward to your comments and feedback.