Skip to main content
Dedaub

Armor.fi ArCore

Smart Contracts Security Assessment

Feb. 24, 2021

Ease

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit on Armor.fi’s arCore set of smart contracts. The arCore modules provide the core functionality for getting insurance plans through Armor, provide balance for these insurance plans, and stake NFT tokens used for insurance.

Four auditors worked on the task over the course of two working weeks. We reviewed the code in significant depth, assessed the economics of the protocol and processed it through automated tools. We also decompiled the code and analyzed it, using our static analysis and symbolic execution tools, to detect possible issues.


SETTING & CAVEATS

The code base is substantial in size, for a total of around 6KLoC, with about 2500 LoC in key new functionality (directories core and general) and a little over that in the periphery (external interfacing and libraries). There is also extra code (e.g., MerkleProof, MerkleDistributor, various liquidity pools) that is not invoked in the core functionality and is not clear whether/how it is currently used. Such code was reviewed only lightly (quick inspection) since we have no integration model with the core functionality. Thus, the emphasis of the audit was on the core functionality (directories core and general). The audit reviewed carefully the rest of the codebase (including, secondarily, the two related repositories, arNFT and arNXM) only to the extent that it is invoked by the core functionality.


The audit focused on security, establishing the overall security model and its robustness and also crypto-economic issues. The latest time of audit was at commit acac2ddf848a73b3115ea53c1e5ccc6807cd0796 of the arCore repo.


TRUST MODEL/CENTRALIZATION ELEMENTS

[This section is included for context, although its contents should already be known to the commissioner of an audit.]

The reviewed protocol uses the blockchain for transparency and proof of transactions (e.g., insurance plan establishment or staking). However, the protocol “owner” needs to be trusted by users. There is no limit to the authority of the protocol owner: they can replace any protocol module, and gain near-full control of user funds, either directly or by subverting mechanisms. This is not limited to funds after replacement but extends to funds held before replacement, since the modules trust each other--e.g., to perform withdrawals. We did not perform a detailed analysis of attack-by-owner, but it is clear that the impact can be nearly-total.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contracts. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

C1

Staking rewards are susceptible to stuffing attacks

C1CRITICAL

Staking rewards are susceptible to stuffing attacks
mitigated

Mitigated

Much longer minimum cover period for staked NFTs

The same linear data structures can be exploited for profit, in ExpireTracker. Staking rewards are susceptible to stuffing attacks, enabling an attacker to capture most of the staking rewards with little upfront investment. An attacker can buy many small insurance contracts (arNFTs that expire within a very short time) and stake them. Note that such arNFTs will cost very little. These arNFTs can be staked in bulk (to reduce gas cost) via the stake manager and will continue to receive staking rewards long after these have expired since these are not removed automatically. The following combined design decisions make this possible:

    • The staking rewards are calculated not on the total price of the contract, but the price of the contract divided by the number of seconds of coverage.
      • The Synthetix-style staking rewards mechanism (RewardsManager.sol) does not take into account the expiration of staked NFTs. Instead, this is relegated to ExpireTraker.sol. In order to remove these expired NFTs from staking (when a high number of NFTs have been staked) requires more gas than staking more NFTs. This may prevent the removal of expired NFTs for significantly longer than their original duration.
        • The “period”, used by RewardsManager.sol is set to 1s. So the attacker can use this low period to guarantee a minimum return.


HIGH SEVERITY

H1

DoS attacks due to unbounded computations

H1HIGH

DoS attacks due to unbounded computations
mitigated

Mitigated

Protocol settings now make attacks much less likely

The linear data structures in BalanceExpireTracker and ExpireTracker can cause unbounded computation and are DoS attack vectors.

There is linear search in a single bucket in function push() (case “// so our nft is somewhere in between”). An attacker can create lots of future entries (from different msg.senders) and immediately drop its balances to zero (or cancel the plans) so as to not get charged. This will mean that no future entries can be entered for that expiry day.

Similarly for staked NFTs in Expire tracker: the linear search within a single bucket can become a DoS attack vector, a gas sink, etc.

We recommended (at the least) a way to externally remove entries so that external monitoring and intervention can remedy the problem.

H2

H2HIGH

Flawed implementation with the linear search
mitigated

Mitigated

3-day buckets used

On the same data structures, there is a linear search backwards by-day. This creates issues, for instance, when a user asks for cover 5 years into the future, this is not the furthest-in-the-future cover, and a search backwards is initiated all the way to, perhaps, recent times. Such a call would need millions of gas to complete.

H3

Incorrect functionality in BalanceManager.keep

H3HIGH

Incorrect functionality in BalanceManager.keep
resolved

In BalanceManager.keep, the call _notifyBalanceChange(msg.sender) should be _notifyBalanceChange(oldHead) for correct functionality.



MEDIUM SEVERITY

[No medium severity issues]


LOW SEVERITY

L1

Possible unintended behavior with calls to EOAs

L1LOW

Possible unintended behavior with calls to EOAs
open

TimelockOwned.executeProposal performs a low-level call without checking extcodesize. Therefore calls to EOAs will succeed by default which is probably not the intended behavior.



OTHER / ADVISORY ISSUES

A1

Better commenting is suggested

A1ADVISORY

Better commenting is suggested
info

The code would benefit from stated high-level invariants, in comments. These help with auditing, but, more importantly, with onboarding other developers. Example invariants include:

  • BalanceManager is the owner of perSecondPrice and balance
  • PlanManager is the owner of plan expiration time
  • ClaimManager is the owner of all staked NFTs
  • Such abstract “ownership” reveals the event-based logic of the code: conceptual updates (and lookups) of any “owned” quantity have to go through the corresponding manager

A2

Possibly insufficient gas for ETH trasnfers

A2ADVISORY

Possibly insufficient gas for ETH trasnfers
info

All modules: (ETH) transfer calls pass 2300 gas and may fail due to that. This is fine, as long as it has been considered.

A3

Unclear functionality

A3ADVISORY

Unclear functionality
info

ArmorMaster: functions addJob, deleteJob are only called externally. Their functionality is not clear.

A4

Outdated comment

A4ADVISORY

Outdated comment
resolved

BalanceManager.withdraw: the comment referring to “Dai” is outdated.

A5

Inconsistent user’s balance records

A5ADVISORY

Inconsistent user’s balance records
resolved

BalanceManager._updateBalance: the code first creates a local copy of the user’s balance record. If the balance is zero, the call to _priceChange sets the user’s balance.perSecondPrice to zero. However, this update is overwritten upon return from _priceChange, by the statement balances[_user] = balance, which restores the local copy. This is surprising behavior, even though we could not see the stale perSecondPrice value causing inconsistencies. The only exception is the emitted event PriceChange(_user, _newPrice), which informs the outside world of a price change that immediately gets overwritten.

A6

Unnecessary casts

A6ADVISORY

Unnecessary casts
resolved

BalanceManager._payPercents: the casts in balances[address(IRewardManager(getModule("REWARD")))] seem unnecessary.

A7

Unconventional path supply for Uniswap swap

A7ADVISORY

Unconventional path supply for Uniswap swap
info

ExchangeManager.buy*: supplying the _path for a Uniswap swap externally is unconventional. In this case, since the caller has to be approved, it seems to merely be an additional element of centralized control, not an attack vector.

A8

No-op checks

A8ADVISORY

No-op checks
info

In multiple modules (BalanceManager, BalanceExpireTracker, ExpireTracker) the directives using SafeMath for uint128 or using SafeMath for uint64 are no-ops, except for subtraction. The library does not check overflow correctly for anything other than uint256. We reviewed all arithmetic and did not find an opportunity for overflow (e.g., all divisions are by constant, all multiplications are preceded by divisions and are used for scaling of non-arbitrary values)

A9

Possible factor out for reusable parts

A9ADVISORY

Possible factor out for reusable parts
resolved

Code such as keccak256(abi.encodePacked("ARMORFI.PLAN.",msg.sender,plans[msg.sender].length - 1,i)) is best factored out into a reusable function. Otherwise a single re-ordering of parameters or misspelling in the constant string can cause a bug.

A10

Incorrect comment

A10ADVISORY

Incorrect comment
resolved

PlanManager.changePrice: the comment says “the new price PER BLOCK”, but the price is per second?

A11

Unused code

A11ADVISORY

Unused code
info

RewardManager.notifyRewardAmount: the code for sending tokens as a reward is currently not exercised: the BalanceManager only supports ETH. This code is from a general SNX staking implementation and seems to be anticipating generalization.

A12

Floating pragmas

A12ADVISORY

Floating pragmas
info

Use of floating pragmas: The floating pragma pragma solidity ^0.6.6;

is used in most contracts, allowing them to be compiled with the 0.6.6 - 0.6.12 versions of the Solidity compiler, while others use pragma solidity ^0.6.0; , making them compilable with any 0.6.x version. Although the differences between these versions are small, floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment.

A13

Compiler bugs

A13ADVISORY

Compiler bugs
info

The contracts were compiled with the Solidity compiler v0.6.12 which has some known minor issues (but relatively few, compared to earlier versions). We have reviewed the issues and do not believe them to affect the contract. More specifically, at the time of writing, there are 2 known compiler bugs associated with the Solidity compiler v0.6.12:

  • Copying an empty bytes or string array from memory to storage can cause data corruption. (We couldn’t find bytes arrays in storage.)

  • Direct assignments of storage arrays with an element size <= 16 bytes (more than one values fit in one 32 byte word) are not correctly cleared if the length of the newly assigned value is smaller than the length of the previous one. (No such array is ever stored.)



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub