Armor.fi ArCore

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.

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.

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.

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

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.

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

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

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

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

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.

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

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.

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)

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.

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

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.

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.

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.)