Inverse Finance contest - neumo's results

Lines of code

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

Vulnerability details

Impact

If a user creates a market with the INVEscrow implementation as escrowImplementation and false as callOnDepositCallback, the deposits made by users in the escrow (through the market) would not mint xINV tokens for them. As callOnDepositCallback is an immutable variable set in the constructor, this mistake would make the market a failure and the user should deploy a new one (even worse, if the error is detected after any user has deposited funds, some sort of migration of funds should be needed).

Proof of Concept

Both escrowImplementation and callOnDepositCallback are immutable:

...
address public immutable escrowImplementation;
...
bool immutable callOnDepositCallback;
...

and its value is set at creation:

constructor (
        address _gov,
        address _lender,
        address _pauseGuardian,
        address _escrowImplementation,
        IDolaBorrowingRights _dbr,
        IERC20 _collateral,
        IOracle _oracle,
        uint _collateralFactorBps,
        uint _replenishmentIncentiveBps,
        uint _liquidationIncentiveBps,
        bool _callOnDepositCallback
    ) {
	...
	escrowImplementation = _escrowImplementation;
	...
	callOnDepositCallback = _callOnDepositCallback;
	...
 }

When the user deposits collateral, if callOnDepositCallback is true, there is a call to the escrow's onDeposit callback:

function deposit(address user, uint amount) public {
	...
	if(callOnDepositCallback) {
		escrow.onDeposit();
	}
	emit Deposit(user, amount);
}

This is INVEscrow's onDeposit function:

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
	}
}

The thing is if callOnDepositCallback is false, this function is never called and the user does not turn his/her collateral (INV) into xINV.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Either make callOnDepositCallback a configurable parameter in Market.sol or always call the onDeposit callback (just get rid of the callOnDepositCallback variable) and leave it empty in case there's no extra functionality that needs to be executed for that escrow. In the case that the same escrow has to execute the callback for some markets and not for others, this solution would imply that there should be two escrows, one with the callback to be executed and another with the callback empty.

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L87 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L121

Vulnerability details

Impact

The price returned by the Oracle is wrong in certain conditions, leading to prices multiplied or divided by 10^n. This situation makes the calculation of the collateral value, the withdrawal limit and the liquidation reward in Market.sol absolutely wrong, which could potentially be a loss of funds for the protocol. See that the price is supposed to be 18 decimals, and getting it in another number of decimals would be a disaster for the protocol. See for example of use of the getPrice function of the oracle in line 326 of Market.sol:

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

how the multiplication is divided by 1 ether (10^18, or one with 18 decimals).

Proof of Concept

In both getPrice and viewPrice functions in Oracle.sol, the error is located when trying to set the price to return as a 18 decimal value.

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
	...
	uint price = feeds[token].feed.latestAnswer();
	require(price > 0, "Invalid feed price");
	// normalize price
	uint8 feedDecimals = feeds[token].feed.decimals();
	uint8 tokenDecimals = feeds[token].tokenDecimals;
	uint8 decimals = 36 - feedDecimals - tokenDecimals;
	uint normalizedPrice = price * (10 ** decimals);
	...
}

Feed decimals is the number of decimals of the Chainlink feed. The price that returns the feed is with feedDecimals so the last line above uint normalizedPrice = price * (10 ** decimals); just should add (18 - feedDecimals) to the price, so that normalizedPrice turns into a 18 decimal value. Instead of this, the functions calculate the decimals as uint8 decimals = 36 - feedDecimals - tokenDecimals; which works for tokenDecimals = 18, but won't work otherwise. So let's see the different possibilities here:

1. If tokenDecimals = 18 the functions will work OK as long as feedDecimals <= 18. If it's greater than 18 the calls to both functions will revert because 36 - 18 - feedDecimals < 0.
2. If tokenDecimals != 18 the calculation won't work OK. Even worse, if tokenDecimals + feedDecimals > 36 both functions will revert.

The following forge test (which I included in the Market.t.sol file) illustrates some wrong returns by the oracle supposing that the feedDecimals are always 18, for different values of tokenDecimals:

function testWrongPriceDecimalsWith18DecimalsFeed() public {
	// The Eth feed has 18 decimals and is supposed to return a price of 1,600 with 18 decimals
	assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e18);

	// We change the feed setting a value of tokenDecimals of 8
	vm.prank(gov);
	oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 8);

	// Check that the price returned by the oracle is now 1,600 with 28 decimals
	assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e28);

	vm.prank(gov);
	// We change the feed again setting a value of tokenDecimals of 24
	oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 24);
	// Check that the call to viewPrice reverts because of the underflow [36 - 18 - 24 = -6]
	vm.expectRevert(stdError.arithmeticError);
	uint price = oracle.viewPrice(address(WETH), collateralFactorBps);
}

To test the cases for when the feed is different than 18 decimals I first changed file EthFeed.sol, precisely these two lines:

...
uint8 decimals_ = 18;
uint price_ = 1600e18;
...

into

...
uint8 decimals_ = 8;
uint price_ = 1600e8;
...

Then added this test to Market.t.sol:

function testWrongPriceDecimalsWith8DecimalsFeed() public {
	// The Eth feed now has 8 decimals, and the token decimals are 18
	// It is supposed to return a price of 1,600 with 18 decimals
	assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e18);

	// We change the feed setting a value of tokenDecimals of 8
	vm.prank(gov);
	oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 8);

	// Check that the price returned by the oracle is now 1,600 with 28 decimals
	assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e28);

	vm.prank(gov);
	// We change the feed again setting a value of tokenDecimals of 30
	oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 30);
	// Check that the call to viewPrice reverts because of the underflow [36 - 8 - 30 = -2]
	vm.expectRevert(stdError.arithmeticError);
	uint price = oracle.viewPrice(address(WETH), collateralFactorBps);
}

So it is clear that the returned price has the wrong decimals in some of the cases, and so the calculations in the Market contract would lead to disastrous consequences.

Tools Used

Forge tests and manual analysis

Recommended Mitigation Steps

If the oracle is supposed to return the price of the feed with 18 decimals, a simple solution would be changing this lines in both viewPrice and getPrice:

uint8 decimals = 36 - feedDecimals - tokenDecimals;
uint normalizedPrice = price * (10 ** decimals);

with this:

uint normalizedPrice = price;
if(feedDecimals <= 18){
	normalizedPrice *= 10 ** (18 - feedDecimals);
}
else{
	normalizedPrice /= 10 ** (feedDecimals - 18);
}

This way, if price feed has less than 18 decimals, it would be right padded with zeroes up to 18, and if it has more than 18 decimals, the excess decimals would be taken out.

1.- Lack of zero address checks:

2.- In BorrowControler.sol, function setOperator should be a two step process.

Function setOperator changes right away the operator of the contract with the address passed as parameter. In case there's an error in the address passed, the contract will not have operator, which can have disastrous consequences, because functions allow and deny will not be usable. Consider turning the change of operator into a two step process, for example with two functions: setPendingOperator and claimOperator.

3.- In Fed.sol, function changeGov should be a two step process.

Function changeGov changes right away the governor of the contract with the address passed as parameter. In case there's an error in the address passed, the contract will not have governor, which can have disastrous consequences, because functions changeSupplyCeiling and changeChair will not be usable. Furthermore, function takeProfit will send the profit to a wrong address. Consider turning the change of governor into a two step process, for example with two functions: setPendingGov and claimGov.

4.- In Market.sol, function setGov should be a two step process.

Function setGov changes right away the governor of the contract with the address passed as parameter. In case there's an error in the address passed, the contract will not have governor, which can have disastrous consequences, because functions setLender, setPauseGuardian, setCollateralFactorBps, setLiquidationFactorBps, setReplenismentIncentiveBps, setLiquidationIncentiveBps, setLiquidationFeeBps and pauseBorrows (when unpausing) will not be usable. Furthermore, function liquidate will send the fee to a wrong address. Consider turning the change of governor into a two step process, for example with two functions: setPendingGov and claimGov.

5.- Centralization risk.

Functions only callable by centralized addresses can have a big impact in user funds.

6.- Immutable variables persist across proxies.

In INVEscrow.sol there is a comment that says // TODO: Test whether an immutable variable will persist across proxies. When the escrow implementation is deployed, the constructor substitutes in the contract bytecode all instances of xINV whith the address supplied, so it is not stored in the storage. This comment can be deleted, because proxies will all see the xINV address supplied at implementation deployment.

7.- escrowImplementation cannot be modified.

If a bug is found in an already deployed escrowImplementation there is no way of changing it because the escrows are not upgradeable and also because there is no function that allows this in the Market.sol contract. Consider creating an onlyGov function setEscrowImplementation to allow the change (this would introduce a new centralization risk, though).

8.- replenishmentIncentiveBps can be set to 0 at construction but not in the setter.

In the Market.sol constructor, variable replenishmentIncentiveBps can be set to zero, but the setter, setReplenismentIncentiveBps does not allow zero as a possible value (the minimum would be then a 0.01% incentive). Consider using the same constraints in both locations, for coherence.