Inverse Finance contest - trustindistrust's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L607-L608

Vulnerability details

Description

The Market.liquidate() function handles liquidations of unhealthy user debt. Anyone can call the function and repay a quantity of the undercollateralized debt position and receive a fee for doing so, paid out of the user's collateral.

When this occurs an additional fee is transferred from the user's collateral to the protocol.

The calculation to do so handles the case when the user's escrow can fully cover the fee. However, it fails to handle the case where the escrow does not contain the required tokens. As such, the fee will not be assessed against the user.

While this may be an intentional design choice, it isn't noted or explained in the documentation.

Severity Rationalization

This would seem to be a value leak at the protocol level. There is no other method by which the protocol could forcefully recover the fee.

If this is an intentional decision to avoid spending gas on transfering (what are anticipated to be) dust amounts of ERC20, it must be pointed out that the liquidator pays this gas cost, not the protocol. Therefore there's no particular reason to not allow the protocol to fully benefit from the liquidation.

Mitigation

Handle the else case in order to capture the fee. Something similar to the following would be sufficient:

if(escrow.balance() >= liquidationFee) { 
    escrow.pay(gov, liquidationFee);
} else {
    escrow.pay(gov, escrow.balance());
}

Tooling

Manual review

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L359 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L376

Vulnerability details

Description

The FiRM Marketplace contract contains multiple governance functions for setting important values for a given debt market. Many of these are numeric values that affect ratios/levels for debt positions, fees, incentives, etc.

In particular, Market.setCollateralFactorBps() sets the ratio for how much collateral is required for loans vs the debt taken on by the user. The lower the value, the less debt a user can take on. See Market.getCreditLimitInternal() for that implementation.

The function Market.getWithdrawalLimitInternal() calculates how much collateral a user can withdraw from the protocol, factoring in their current level of debt. It contains the following check:

if(collateralFactorBps == 0) return 0;

This would cause the user to not be able to withdraw any tokens, so long as they had any non-0 amount of debt and the collateralFactorBps was 0.

Severity Rationalization

It is the warden's estimation that all semantics for locking functionality of the protocol should be explicit rather than implicit. While it is very unlikely that governance would intentionally set this value to 0, if it were to do so it would disproportionately affect users whose debt values were low compared to their deposited collateral.

It is also obvious that the same function that set the value to 0 could be used to revert the change. However, this would take time. Inverse Finance has mandatory minimums for the time required to process governance items in its workflow (https://docs.inverse.finance/inverse-finance/governance/creating-a-proposal)

The community has a social agreement to post all proposals on the forum and as a draft in GovMills at least 24 hours before the proposal is put up for an on-chain vote, and also to host a community call focusing on the proposal before the voting period.

Once a proposal has passed, it must be queued on-chain. This action can be triggered by anyone who is willing to pay the gas fee (usually done by a DAO member). The proposal then enters a holding period of 40 hours to allow users time to prepare for the consequences of the execution of the proposal.

As such, were the situation to occur it would cause at least 64 hours of lock.

Since the contract itself only overtly contains locking for new borrowing, this implicit lock on withdraws seems like an unnecessary risk.

Mitigation

Consider a minimum for this value, to go along with the maximum value check already present in the setter function. While this will still reduce the quantity of collateral that can be withdrawn by users, it would allow for some withdraws to occur.

An explicit withdrawal lock could be implemented, making the semantic clear. This function could have modified access controls to enable faster reactions vs governance alone.

Alternatively, if there was an intention for this value to accept 0, consider an 'escape hatch' function that could be enacted by users when a 'defaulted' state is set on the Market.

Tooling

Manual review

Non-critical

Contract is missing events for certain actions

Locations 1 2 3 4 5 6 7 8 9 10 11 12 13 14

Description

Sensitive actions like access control changes and ownership on contracts should emit events for easy tracking and notification.

Suggested course of action

Implement new events for functions that should have them.

Low

Unorthodox token decimals normalization algorithm limits maximum token precision and introduces operational risk

Locations: 1 2

Description

The Oracle contract handles the recording of pricing data for the lending system. Other contracts utilize viewPrice() and getPrice() to fetch pricing data.

These functions include the following lines:

uint8 feedDecimals = feeds[token].feed.decimals();
uint8 tokenDecimals = feeds[token].tokenDecimals;
uint8 decimals = 36 - feedDecimals - tokenDecimals;
uint normalizedPrice = price * (10 ** decimals);

tokenDecimals comes from data supplied to the setter function setFeed() as an argument set by the operator. feedDecimals comes from data supplied by the Chainlink infrastructure.

As such there are a couple of potential issues.

1. It's unclear from the code why two 'samples' of the token's precision are being taken during normalization. If the operator supplies an incorrect value via setFeed(), the normalized price could be off by one or more orders of magnitude. The gain compared to the operational risk is unclear.
2. Due to the subtraction, assuming the two values are equal, the protocol is limiting itself to tokens that use at most 18 decimals of precision. While this is the overwhelming majority of tokens, this makes the contract somewhat inflexible against future developments.

Suggested course of action

Consider the business case for allowing the operator to supply a token precision that diverges from the Chainlink-supplied value. If the intention is to tacitly support other oracles that might not directly supply precision values, carefully weigh the risk of such oracles later adding such data to the operation of the protocol oracle.

Low

​

DBR token lacks address(0) guards on transfer functions while possessing public burn function

​

Description

​ The DolaBorrowingRights contract implements most ERC20 semantics, including transfer functions and mint/burn functions. ​ However, while burn() is public and emits an Event for burning, the transfer() and transferFrom() functions lack a check to block 'burning' of tokens by sending them to the 0 address. ​

Suggested course of action

​ Modify the transfer functions to that it's clear that burning tokens is handled via burn() and so that the correct intentioned events are emitted.