KeeperDAO Audit

audit.md

Overview

A four person-day audit was performed on KeeperDAO's protocol. In total, 2 undetermined, 2 high, 2 low, and 5 informational findings were documented. Additionally, 7 recommendations were made.

Scope

Contracts (excluding mocks) located within the following repositories at the specified commit hash were included in the audit.

Repository	Commit Hash
keeperdao/protocol	8edbc65a2227ecb49b1ac0d62adc239f67dc4bcd
keeperdao/token	0b659e993d844e68e85f7655f9a8e4157371e5f6
keeperdao/liquiditypool	72c3ccabde796780a7fd0f3e15d756561842ed25
keeperdao/keeper	2f9c043a759594da348adaba776f64056aea3da6
keeperdao/compound	0e709ac08c9da7349d233e578058b5ba50dd5295
keeperdao/rebalancer	d1eb2c229d316edd23f57039240be2240f48bd80
keeperdao/feecollector	f91c98390cecb60fda7356d0b3ffbdf472836e00

Summary of Findings

Undetermined severity

The risk of these findings are unquantifiable, but may be significant enough to warrant attention.

• Solidity optimizer is enabled
• Solidity versions are mismatched and out of date

High severity

These findings could significantly affect the operation of the project, do not require elevated privileges, and cannot be mitigated after deployment.

• KyberNetworkRebalancer causes liquidations at high gas prices to fail
• Attackers can steal profits from KyberNetworkRebalancer

Low severity

These findings could affect the operation of the project, may require elevated privileges, and cannot be mitigated after deployment.

• Functions are missing return statements
• Destroying a SingleAssetLiquidityPool does not appear to behave correctly

Informational

These findings could affect the operation of the project, require elevated privileges, and may be mitigated after deployment.

• KyberNetworkRebalancer will fail to consolidate if fee or profit is zero
• KToken balance is not tied to token ownership in Keeper
• Entering and exiting Compound markets may fail
• Owners are able to change dangerous settings
• Functions may be vulnerable to reentrancy

Recommendations

These findings do not represent risk to the project, but the project may still benefit from addressing them.

• Consider exposing recoverableTokensBlacklist in CanReclaimTokens
• Consider emitting an event when a token is blacklisted
• Consider optimizing gas usage during consolidation
• Consider correcting typos
• Consider using wrapped ethereum instead of ethereum
• Consider using SafeMath in additional locations
• Consider applying the checks-effects-interactions pattern in additional locations

Findings

Solidity optimizer is enabled

Finding ID	Severity	Related	Resolution
KD-0001	Undetermined

Description

Currently, the Solidity optimizer is enabled for all repositories with the default runs value of 200.

The optimizer may introduce unexpected bugs which can only be detected through rigorous testing, formal verification, manual inspection of the generated bytecode, or some other technique. This unquantifiable risk may be undesirable depending on the exact magnitude of gas saved.

However, if the risk is considered to be acceptable, then it should be noted that the runs setting does not necessarily specify the number of passes that the optimizer will make. Rather, it is defined as the number of times that each instruction is expected to be executed, or in other words, the number of expected transactions.

Remediation

If the gas saved is not significant, consider disabling the Solidity optimizer. Alternatively, if optimization is desired, consider changing the value of the runs setting to more accurately reflect the expected number of transactions.

Solidity versions are out-of-date and inconsistent

Finding ID	Severity	Related	Resolution
KD-0002	Undetermined

Description

The Solidity versions used by each repository are listed below:

Repository	Version
keeperdao/protocol	0.5.8
keeperdao/token	0.5.8
keeperdao/liquiditypool	0.5.8
keeperdao/keeper	0.5.12
keeperdao/compound	0.5.12
keeperdao/rebalancer	0.5.8
keeperdao/feecollector	0.5.8

While recently released versions of Solidity have not been battle-tested and may contain bugs, 0.5.15 was released in December and contains several bugfixes. Furthermore, while the current codebase doesn't appear to be affected by these bugs, that may not be the case with future upgrades. Finally, using multiple versions of Solidity exposes the protocol to more risk than using a single version.

Remediation

Consider upgrading all repositories to use Solidity version 0.5.15, or standardizing on a single version of Solidity.

KyberNetworkRebalancer causes liquidations at high gas prices to fail

Finding ID	Severity	Related	Resolution
KD-0003	High

Description

The KyberNetwork contract requires that the gas price of any transaction interacting with it be less than a configurable value, currently set to 50 gwei:

function trade(TradeInput tradeInput) internal returns(uint) {
    require(isEnabled);
    require(tx.gasprice <= maxGasPriceValue);

Kyber most likely introduced this limitation to protect their traders from sandwich attacks, where an attacker sends two transactions which sandwich the trader's transaction and make it less profitable.

However, KeeperDAO presumably would not want to be constrained in terms of gas price. Liquidation is competitive, with bots bidding by increasing the gas price of their transaction. A gas price cap imposed by Kyber would artificially cripple KeeperDAO's ability to keep up with the competition, directly impacting profits.

Remediation

It may be possible to negotiate with the Kyber team to have KeeperDAO be exempt from the gas price limitations. Failing that, simply documenting this limitation or introducing a Rebalancer which can rebalance collateral on a different exchange when gas prices are too high may be an alternative.

Attackers can steal profits from KyberNetworkRebalancer

Finding ID	Severity	Related	Resolution
KD-0004	High

Description

Recent events have shown that attacks which profit from slippage can be rather lucrative. The KyberNetworkRebalancer is vulnerable to a form of this attack during consolidation of leftover collateral (i.e. profit)

An attacker can write a smart contract which uses flash loans to depress the exchange rate between _leftoverToken and assetToken, before triggering a consolidation on the KyberNetworkRebalancer. This will allow the attacker to essentially extract the profits from the KyberNetworkRebalancer for themselves.

Remediation

The KeeperDAO team could choose to restrict consolidation to EOA addresses, or to simply make consolidation a privileged action. Alternatively, the KeeperDAO team could choose to run a bot which attempts to automatically consolidate funds, although that bot is still at risk of being frontrun. Finally, the KeeperDAO team could simply accept this as a known limitation of the protocol.

Functions are missing return statements

Finding ID	Severity	Related	Resolution
KD-0005	Low

Description

The following functions are missing return statements.

• Compound#unbalancedBalance
• Compound#take
• Compound#withdraw
• LiquidityPool#withdraw

Remediation

Ensure that the functions are returning a value, or remove the returns clause from the function definition.

Destroying a SingleAssetLiquidityPool does not appear to behave correctly

Finding ID	Severity	Related	Resolution
KD-0006	Low

Description

When a SingleAssetLiquidityPool is destroyed, it tries to transfer its position as operator on the Keeper to the sender. However, only the owner has the ability to do this. Therefore, one of two outcomes is possible:

1. The call will fail because the SingleAssetLiquidityPool is not the owner of the Keeper
2. The call will succeed and ownership of the Keeper is lost forever

Neither of these outcomes appear to be ideal.

Remediation

Allow operators to transfer operator permissions on SingleLenderKeeper, or transfer ownership after transferring operator permissions to ensure ownership is not burnt.

KyberNetworkRebalancer will fail to consolidate if fee or profit is zero

Finding ID	Severity	Related	Resolution
KD-0007	Informational

Description

When consolidating leftover collateral, the KyberNetworkRebalancer will consult the FeeCollector instance for the fee to charge. If this fee is either zero, or the entirety of the collateral, then the entire transaction will revert.

This is because Kyber does not support performing swaps where the src token amount is zero:

function validateTradeInput(ERC20 src, uint srcAmount, ERC20 dest, address destAddress)
    internal
    view
    returns(bool)
{
    require(srcAmount <= MAX_QTY);
    require(srcAmount != 0);
    require(destAddress != address(0));
    require(src != dest);

Remediation

Check if the value to be exchanged is zero, and don't perform the exchange if it is the case.

KToken balance is not tied to token ownership in Keeper

Finding ID	Severity	Related	Resolution
KD-0008	Informational

Description

Suppose there currently exists some Lender instance which contains some funds, as well as a corresponding Keeper. Now suppose that a new LiquidityPool is deployed and paired with the existing Keeper.

A user deposits some funds via the new LiquidityPool. Because the total supply of the KToken is zero, the user is minted TOKEN_MULTIPLIER * _amount tokens.

The user then immediately withdraws the tokens they were just minted. First, the scaled ratio must be calculated.

uint256 scaledRatio = _state.keeper.ratioScale().mul(_amountOfKTokens).div(_state.kToken.totalSupply());
// scaledRatio = _state.keeper.ratioScale()

Now, the withdraw amount must be calculated.

function valueOf(address _token, uint256 _scaledRatio) external returns (uint256) {
    return lender.balanceOf(_token).mul(_scaledRatio).div(state.ratioScale);
}

uint256 withdrawAmount = _state.keeper.valueOf(address(_state.assetToken), scaledRatio);
// withdrawAmount = lender.balanceOf(address(_state.assetToken))

We can see that the user will end up withdrawing the entire balance of the Lender, despite not having deposited that amount. This is because the number of tokens minted is not based on the value of the user's deposit relative to the Lender's balance.

Remediation

Disallow the changing of operators on a Keeper instance such that this cannot be exploited by an administrator, or devise an alternative issuance algorithm which accounts for this situation

Entering and exiting Compound markets may fail

Finding ID	Severity	Related	Resolution
KD-0009	Informational

Description

When entering a market on Compound, the operation may fail if the supplied CToken is unlisted or if the sender has entered too many markets already.

Similarly, exiting a market may fail if the sender owes money in the market or would not have enough collateral after exiting the market.

While it's possible for the Compound lender to recover from any failed enters or exits, it would be safer to assert that the operation succeeded before making any state changes.

Remediation

Wrap the calls to Comptroller#enterMarkets and Comptroller#exitMarket with requireNoError

Owners are able to change dangerous settings

Finding ID	Severity	Related	Resolution
KD-0010	Informational

Description

Owners have the ability to change a variety of state variables, but some of the variables shouldn't need to be changed and makes the owner key riskier to hold.

The following fields could probably be made immutable:

Field	Reasoning
Compound#gastoken	The GasToken contracts have existed for a while now, and there probably won't be a need to replace them
SingleAssetLiquidityPool#assetToken	Given that the pool should only hold a single asset, it seems like in all cases where the token needs to be replaced, it would be better to deploy a new pool instead
SingleAssetLiquidityPool#kToken	A new instance of KToken is created at deploy-time, there doesn't seem to be a need to ever change this

Remediation

Remove the ability for the owner to change these values

Functions may be vulnerable to reentrancy

Finding ID	Severity	Related	Resolution
KD-0011	Recommendation	KD-0017

Description

Reentrancy can cause unpredictable behavior in otherwise sound contracts. Some contracts in the KeeperDAO protocol interact with external contracts whose implementations may allow for reentrancy.

The following functions may benefit from reentrancy protection:

Function	Reasoning
Compund#liquidate	This function involves movement of protocol funds to an attacker-accessible location and triggers a transfer call on an ERC20 token. This call could potentially trigger a callback, if for example the token also implemented ERC777.
SingleAssetLiquidityPool#withdraw	This function involves movement of protocol funds to an attacker and triggers a transfer call on an ERC20 token

Remediation

Apply defense in depth and use OpenZeppelin's ReentrancyGuard.sol to prevent the possibility of reentrancy attacks.

Consider exposing recoverableTokensBlacklist in CanReclaimTokens

Finding ID	Severity	Related	Resolution
KD-0012	Recommendation

Description

Users may be interested in being able to verify whether a token is blacklisted, either through a contract, or through the Etherscan "Read Contract" view.

Remediation

Set the visibility of the field to public

Consider emitting an event when a token is blacklisted

Finding ID	Severity	Related	Resolution
KD-0013	Recommendation

Description

Users may be interested in compiling a list of blacklisted tokens off-chain, but at the moment that involves non-trivial tracing of all transactions which may interact with the protocol.

Remediation

Define and emit an event, such as event TokenBlacklisted(address indexed token)

Consider optimizing gas usage during consolidation

Finding ID	Severity	Related	Resolution
KD-0014	Recommendation

Description

The average Kyber exchange uses about 300,000 gas. By making one trade instead of two trades during consolidation, significant gas savings can be obtained.

Remediation

Perform one swap for the entirety of leftoverCollateral and manually transfer the assetToken to the feeCollector and state.keeper

Consider correcting typos

Finding ID	Severity	Related	Resolution
KD-0015	Recommendation

Description

The following files contain typos

Files	Typo
KToken.sol#L59-69	underlyingTokenName is misspelled as underlyngTokenName
KToken.sol#L71-78	underlyingTokenSymbol is misspelled as underlyngTokenSymbol
KyberNetworkRebalancer.sol#L27	contract is misspelled as cotract
Rebalancer.sol#L12	converted is misspelled as conveted

Remediation

Correct typos

Consider using wrapped ethereum instead of ethereum

Finding ID	Severity	Related	Resolution
KD-0016	Recommendation

Description

There's a non-trivial amount of code centered around handling ETH as a special case, and code complexity increases the risk of bugs.

Remediation

Consider using WETH instead of ethereum. The ETH can be wrapped in entrypoints for the sake of user convenience and unwrapped in any lenders which don't accept WETH. This reduces complexity when flowing value from users to lenders. Note that Kyber already supports trading WETH

Consider applying the checks-effects-interactions pattern in additional locations

Finding ID	Severity	Related	Resolution
KD-0017	Recommendation	KD-0011

Description

The checks-effects-interactions pattern was developed to reduce the risk associated with reentrancy. While not a panacea, following the CEI pattern is usually better than not.

The following functions may benefit from following the CEI pattern:

Function	Reasoning
SingleAssetLiquidityPool#withdraw	This function interacts with the trusted KToken after affecting external state by causing an ERC20 transfer

Remediation

Apply the CEI pattern where reasonable

Consider using SafeMath in additional locations

Finding ID	Severity	Related	Resolution
KD-0018	Recommendation

Description

OpenZeppelin's SafeMath is a cornerstone of safe smart contract development. Sometimes it's hard to guarantee that an operation will always be safe. In those cases, it may be better to err on the side of using SafeMath.

Some operations within KeeperDAO could benefit from the usage of SafeMath:

Line number	Explanation
KyberNetworkRebalancer#L156	While the fee collector is a privileged contract, the caller has no guarantees that the returned value will be less than than the input value
LiquidityPool#L64	The _amount variable is technically user-controlled, even if in practice it's very unlikely this operation may overflow

Resolution

Use SafeMath in the locations indicated