Inverse Finance contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

When user deposit in the market.sol, if the callOnDepositCallback is turned on. but underlying escrow does not support onDeposit hook. transaction revert.

function deposit(address user, uint amount) public {
	IEscrow escrow = getEscrow(user);
	collateral.transferFrom(msg.sender, address(escrow), amount);
	if(callOnDepositCallback) {
		escrow.onDeposit();
	}
	emit Deposit(user, amount);
}

In the current implementation, only one escrow implementation support onDeposit hook, the other two does not.

INVEscrow.sol support onDeposit

function onDeposit() public {
	uint invBalance = token.balanceOf(address(this));
	if(invBalance > 0) {
		xINV.mint(invBalance); // we do not check return value because we don't want errors to block this call
	}
}

In GovTokenEscrow and SimpleERC20Escrow.sol, onDeposit hook is not supported. the funciton is commented.

    /* Uncomment if Escrow contract should handle on deposit callbacks. This function should remain callable by anyone to handle direct inbound transfers.
    function onDeposit() public {

    }
    */

If GovTokenEscrow and SimpleERC20Escrow.sol is set as implementation, and callOnDepositCallback is set to True, transaction revert.

This is more damaging than callOnDepositCallback is to False, and implementation address is INVEscrow.sol because the onDeposit would just not called. transaction does not revert.

Proof of Concept

We change the setting from

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

initialize(replenishmentPriceBps, collateralFactorBps, replenishmentIncentiveBps, liquidationBonusBps, callOnDepositCallback);

to

initialize(replenishmentPriceBps, collateralFactorBps, replenishmentIncentiveBps, liquidationBonusBps, true);

We run

forge test -vvv --match testDeposit2

the log is

Running 1 test for src/test/Market.t.sol:MarketTest
[FAIL. Reason: EvmError: Revert] testDeposit2() (gas: 219281)
Traces:
  [219281] MarketTest::testDeposit2() 
    ├─ [0] VM::deal(user: [0x0000000000000000000000000000000000000069], 1000000000000000000)
    │   └─ ← ()
    ├─ [0] VM::startPrank(user: [0x0000000000000000000000000000000000000069])
    │   └─ ← ()
    ├─ [23914] WETH9::deposit{value: 1000000000000000000}()
    │   ├─ emit Deposit(account: user: [0x0000000000000000000000000000000000000069], amount: 1000000000000000000)
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [541] WETH9::balanceOf(user: [0x0000000000000000000000000000000000000069]) [staticcall]
    │   └─ ← 1000000000000000000
    ├─ [0] VM::startPrank(user: [0x0000000000000000000000000000000000000069])
    │   └─ ← ()
    ├─ [24523] WETH9::approve(Market: [0xd812a1705b5bea2963122be3e45e8ca4cc5f7f78], 1000000000000000000)
    │   ├─ emit Approval(owner: user: [0x0000000000000000000000000000000000000069], spender: Market: [0xd812a1705b5bea2963122be3e45e8ca4cc5f7f78], value: 1000000000000000000)
    │   └─ ← true
    ├─ [112941] Market::deposit(user2: [0x0000000000000000000000000000000000000070], 1000000000000000000)
    │   ├─ [9031] → new <Unknown>@"0x5de8…db4a"
    │   │   └─ ← 45 bytes of code
    │   ├─ emit CreateEscrow(user: user2: [0x0000000000000000000000000000000000000070], escrow: 0x5de82d50380e2072312b5902f6dd372e78d0db4a)
    │   ├─ [47491] 0x5de8…db4a::initialize(WETH9: [0x114c28ea2cde99e41d2566b1cfaa64a84f564caf], user2: [0x0000000000000000000000000000000000000070])
    │   │   ├─ [44816] SimpleERC20Escrow::initialize(WETH9: [0x114c28ea2cde99e41d2566b1cfaa64a84f564caf], user2: [0x0000000000000000000000000000000000000070]) [delegatecall]
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   ├─ [21131] WETH9::transferFrom(user: [0x0000000000000000000000000000000000000069], 0x5de82d50380e2072312b5902f6dd372e78d0db4a, 1000000000000000000)
    │   │   ├─ emit Transfer(from: user: [0x0000000000000000000000000000000000000069], to: 0x5de82d50380e2072312b5902f6dd372e78d0db4a, value: 1000000000000000000)
    │   │   └─ ← true
    │   ├─ [354] 0x5de8…db4a::onDeposit()
    │   │   ├─ [192] SimpleERC20Escrow::onDeposit() [delegatecall]
    │   │   │   └─ ← "EvmError: Revert"
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; finished in 4.27ms

Failing tests:
Encountered 1 failing test in src/test/Market.t.sol:MarketTest
[FAIL. Reason: EvmError: Revert] testDeposit2() (gas: 219281)

Encountered a total of 1 failing tests, 0 tests succeeded

clearly it shows that the transaciton in onDeposit() revert because

the escrow imiplmenetation is just SimpleERC20Escrow.sol, which does not support onDeposit

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

We recommend the project uncomment the onDeposit in GovTokenEscrow and SimpleERC20Escrow, even at least leave them empty.

    function onDeposit() public {
			 // emit event or do nothing
    }

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L323 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L326 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L353 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L360 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L389 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L394 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L461 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L594

Vulnerability details

Impact

In the current implementation, the stale / oudated / old price can be used in the system, because the oracle failed to check Chainlink updatedAt timestamp in Oracle.sol.

Oracle is considered a cruical part of the architecture in the current codebase.

Because the oracle price is used to determine how much DOLA token user can borrow against collateral.

If the oracle is not functioning, the user can overborrow and result in project insolvency.

In Oracle.sol, function viewPrice view the price, while getPrice return price and save the price (update the internal price)

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
function getPrice(address token, uint collateralFactorBps) external returns (uint) {

When user borrow in Market.sol, they call function borrowInternal

uint credit = getCreditLimitInternal(borrower);
debts[borrower] += amount;
require(credit >= debts[borrower], "Exceeded credit limit");

the function getCreditLimitInternal (borrower) calls

function getCreditLimitInternal(address user) internal returns (uint) {
	uint collateralValue = getCollateralValueInternal(user);
	return collateralValue * collateralFactorBps / 10000;
}

which calls

return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether;

the crucial oracle.getPrice call

uint price = feeds[token].feed.latestAnswer();
require(price > 0, "Invalid feed price");

the chainlink only fetches the "lastest answer" without checking the when the answer is updated in the chainlink side.

The stale price impacts withdraw:

function withdrawInternal(address from, address to, uint amount) internal {
	uint limit = getWithdrawalLimitInternal(from);
	require(limit >= amount, "Insufficient withdrawal limit");

the code above may think that user have enough credit limit and user can over-withdraw if the stale price is used.

The stale price also impacts liquidation:

function liquidate(address user, uint repaidDebt) public {
	require(repaidDebt > 0, "Must repay positive debt");
	uint debt = debts[user];
	require(getCreditLimitInternal(user) < debt, "User debt is healthy");

if the price drop but the price is not updated from chainlink, the default asset can become not liquidable because the code above think that the user

has enough credit limit.

require(getCreditLimitInternal(user) < debt, "User debt is healthy");

Proof of Concept

1. The project set the chainlink price feed WETH / TokenA.
2. The project create a market.
3. The latest up-to-date price is 1000 ETHER (unit).
4. User is capable of borrowing token at price level 1000 ETHER (unit)
5. chainlink data feed become stale and not updating. The real price drop to 500 ETHER (unit)
6. The chainlink latest price still return 1000 ETHER (unit), it is the latest price, but a old / outdated / stale one.
7. User continue to overborrow, put the project in a insolvent state.

Tools Used

Manual Review

Recommended Mitigation Steps

Source: https://docs.chain.link/docs/data-feeds/#check-the-timestamp-of-the-latest-answer

We recommend the project check the timestamp of the latest answer.

Quote from chainlink:

Your application should track the latestTimestamp variable or use the updatedAt value from the latestRoundData() function to make sure that the latest answer is recent enough for your application to use it. If your application detects that the reported answer is not updated within the heartbeat or within time limits that you determine are acceptable for your application, pause operation or switch to an alternate operation mode while identifying the cause of the delay.

During periods of low volatility, the heartbeat triggers updates to the latest answer. Some heartbeats are configured to last several hours, so your application should check the timestamp and verify that the latest answer is recent enough for your application.

Also instead of using

Interface IChainlinkFeed {
	function decimals() external view returns (uint8);
	function latestAnswer() external view returns (uint);
}

we recommend the project use priceFeed.latestRoundData(),

which returns the updated timestamp and check if the updated timestamp is too far away from the current timestamp.

https://github.com/smartcontractkit/chainlink/blob/72a72aa083ce864b79b8be6e2a2cdb615302c412/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol#L11

function getRoundData(uint80 _roundId)
external
view
returns (
  uint80 roundId,
  int256 answer,
  uint256 startedAt,
  uint256 updatedAt,
  uint80 answeredInRound
);