Fungify Pools Contracts Security Review Report

The updating of the token balance of a user should be accompanied by the updating of the supply interest index and accrued of the user.

If not, the interest rewards to-be-claimed can be subject to manipulation by increasing the balance before calculation, resulting in a greater amount of rewards depending on the balance and time span.

Similar to “FNG-11: All interest tokens can be stolen from the interest market”, this issue still exists in the CErc721.sol:_seize where the collateral of the borrower is transferred to the liquidator through directly updating the accountTokens mapping. The function does not update the supplyInterest (which is now only done upon normal transfers).

As a result, the token balance of the liquidator increases before the interest rewards calculation and so the new balance will also be counted over the differences in index and time compared to the last time it was updated.

This can be exploited by having some built up interest from the index and enough accounts (e.g. using an exploit contract) to completely drain the interest market from all its tokens, as it can be used to fabricate rewards instantly and those can be collected from the interest market in its underlying tokens.

 function _seize(address liquidator, address borrower, uint seizeTokens) override external nonReentrant returns (uint) {
 if (msg.sender != address(comptroller)) {
 revert Unauthorized();
 }
 accrueInterest();
 uint oneNFTAmount = doubleScale / exchangeRateStoredInternal();
 if (seizeTokens % oneNFTAmount != 0) {
 // ensure whole nft seize size by rounding up to the next whole NFT
 seizeTokens = ((seizeTokens / oneNFTAmount) + 1) * oneNFTAmount;
 }
 /* Fail if borrower = liquidator */
 if (borrower == liquidator) {
 revert LiquidateSeizeLiquidatorIsBorrower();
 }
 uint liquidatorSeizeTokens = seizeTokens;
 /////////////////////////
 // EFFECTS & INTERACTIONS
 // (No safe failures beyond this point)
 /* We write the calculated values into storage */
 accountTokens[borrower] = accountTokens[borrower] - seizeTokens;
 accountTokens[liquidator] = accountTokens[liquidator] + liquidatorSeizeTokens;
 /* Emit a Transfer event */
 emit Transfer(borrower, liquidator, liquidatorSeizeTokens);
 //emit Transfer(borrower, address(this), protocolSeizeTokens);
 //emit ReservesAdded(address(this), protocolSeizeAmount, totalReservesNew);
 return seizeTokens;
 }

The remediation is similar to “FNG-11: All interest tokens can be stolen from the interest market”. The supplyInterest mapping should be updated for both users, before moving balances.

For example:

 function _seize(address liquidator, address borrower, uint seizeTokens) override external nonReentrant returns (uint) {
 if (msg.sender != address(comptroller)) {
 revert Unauthorized();
 }
 accrueInterest();
>>>> supplyInterest[borrower].interestAccrued = supplyInterestStoredInternal(src);
>>>> supplyInterest[borrower].interestIndex = supplyIndex;
>>>> supplyInterest[liquidator].interestAccrued = supplyInterestStoredInternal(dst);
>>>> supplyInterest[liquidator].interestIndex = supplyIndex;
 uint oneNFTAmount = doubleScale / exchangeRateStoredInternal();
 if (seizeTokens % oneNFTAmount != 0) {
 // ensure whole nft seize size by rounding up to the next whole NFT
 seizeTokens = ((seizeTokens / oneNFTAmount) + 1) * oneNFTAmount;
 }
 /* Fail if borrower = liquidator */
 if (borrower == liquidator) {
 revert LiquidateSeizeLiquidatorIsBorrower();
 }
 uint liquidatorSeizeTokens = seizeTokens;
 /////////////////////////
 // EFFECTS & INTERACTIONS
 // (No safe failures beyond this point)
 /* We write the calculated values into storage */
 accountTokens[borrower] = accountTokens[borrower] - seizeTokens;
 accountTokens[liquidator] = accountTokens[liquidator] + liquidatorSeizeTokens;
 /* Emit a Transfer event */
 emit Transfer(borrower, liquidator, liquidatorSeizeTokens);
 //emit Transfer(borrower, address(this), protocolSeizeTokens);
 //emit ReservesAdded(address(this), protocolSeizeAmount, totalReservesNew);
 return seizeTokens;
 }

The batchLiquidateBorrow function in Comptroller.sol calculates the amount of funds that will be transferred in a liquidation. This computation relies on asset prices retrieved from an oracle through the oracle_.getUnderlyingPrice call. However, it's important to highlight the absence of slippage protection in this process.

There's no guarantee that the amount returned by the batchLiquidateBorrow function corresponds to the current market price. This is because the transaction that updates the price feed might be mined before the actual call to batchLiquidateBorrow. As a result, users may experience liquidation amounts that differ from their expectations.

Consider a scenario where a user initiates a batchLiquidateBorrow function. Due to an oracle malfunction or significant price fluctuations, the amount of collateral transferred from the module might be much lower than the user would have anticipated based on the current market conditions.

It is recommended to introduce a parameter that would empower the caller to set a threshold for the minimum acceptable value for the liquidation. By doing so, users can explicitly state their assumptions about the liquidation and ensure that the collateral payout remains as profitable as expected.

 function batchLiquidateBorrow(address borrower, Liquidatables[] memory liquidatables, CTokenInterface[] memory
cTokenCollaterals) external nonReentrant returns (uint[][2] memory results) {
 require(adminWhitelist[msg.sender], "unauthorized");
 require(!seizeGuardianPaused, "seize is paused");
 (uint err, uint beforeRatio) = getAccountDebtRatio(borrower);
 require(err == uint(Error.NO_ERROR) && beforeRatio != 0, "INSUFFICIENT_SHORTFALL");
 results[0] = new uint[](liquidatables.length);
 results[1] = new uint[](cTokenCollaterals.length);
 PriceOracle oracle_ = oracle;
 uint liquidatedValueTotal;
 {
 for (uint i = 0; i < liquidatables.length;) {
 require(markets[liquidatables[i].cToken].isListed, "market not listed");
 if (liquidatables[i].amount != 0) {
 results[0][i] = CErc20Interface(liquidatables[i].cToken)._liquidateBorrow(msg.sender, borrower, liquidatables[i].amount);
 } else {
 results[0][i] = CErc721Interface(liquidatables[i].cToken)._liquidateBorrow(msg.sender, borrower, liquidatables[i].nftIds);
 }
 uint priceMantissa = oracle_.getUnderlyingPrice(CToken(liquidatables[i].cToken));
 require(priceMantissa != 0, "PRICE_ERROR");
 liquidatedValueTotal = mul_ScalarTruncateAddUInt(Exp({mantissa: priceMantissa}), results[0][i], liquidatedValueTotal);
 unchecked { i++; }
 }
 require(liquidatedValueTotal != 0, "error");
 }

 {
 uint liquidatedValueRemaining = liquidatedValueTotal;
 for (uint i = 0; i < cTokenCollaterals.length && liquidatedValueRemaining != 0;) {
 require(markets[address(cTokenCollaterals[i])].isListed, "market not listed");
 {
 /* We calculate the number of collateral tokens that will be seized */
 uint seizeTokens = liquidateCalculateSeizeTokensNormed(address(cTokenCollaterals[i]), liquidatedValueRemaining);
 uint actualSeizeTokens;
 uint borrowerBalance = cTokenCollaterals[i].balanceOf(borrower);
 if (borrowerBalance < seizeTokens) {
 // can't seize more collateral than owned by the borrower
 actualSeizeTokens = borrowerBalance;
 } else {
 actualSeizeTokens = seizeTokens;
 }
 actualSeizeTokens = cTokenCollaterals[i]._seize(msg.sender, borrower, actualSeizeTokens);
 require(actualSeizeTokens != 0, "token seizure failed");
 uint actualRepayAmount = liquidatedValueRemaining;
 if (actualSeizeTokens != seizeTokens) {
 actualRepayAmount = actualRepayAmount * actualSeizeTokens / seizeTokens;
 }
 liquidatedValueRemaining = liquidatedValueRemaining > actualRepayAmount ?
 liquidatedValueRemaining - actualRepayAmount :
 0;
 results[1][i] = actualSeizeTokens;
 }
 unchecked { i++; }
 }
 require(liquidatedValueRemaining == 0, "LIQUIDATE_SEIZE_TOO_LITTLE");
 }

 /* We emit a LiquidateBorrow event */
 //emit LiquidateBorrow(liquidator, borrower, nftIds, repayInterest, cTokenCollaterals, seizeTokensList);
 uint afterRatio;
 (err, afterRatio) = getAccountDebtRatio(borrower);
 
 // we allow Error.TOO_LITTLE_INTEREST_RESERVE here as long as debt ratio is improved or unchanged
 require((err == uint(Error.NO_ERROR) || err == uint(Error.TOO_LITTLE_INTEREST_RESERVE))
 && afterRatio <= beforeRatio, "seize too much");
 return results;
 }

Add e.g. minimumLiquidatedValue parameter and add a check verifying that the reward value is greater than or equal to the minimumLiquidatedValue value.