FintruX Network - Audit

The function `_getCurrentCacheAmounts()` internally calls `getPendingETHReward()` to obtain the pending eth reward for `_borrower`, which is then added to `currentETH` with the variable `Caches[_borrower].coll`. However, on line 950 line, another pending reward is also added to `currentETH`. If a reward exists for that borrower, then the borrower's collateral rate would be impacted. For example, let's say the `pendingETHLoss` is greater than 0. Then `pendingETHLoss` is equal to `pendingETHReward`. This value is first added to `currentETH` on L945, and it is added to `currentETH` for the second time on L953. So there are a total of two rewards accrued for the borrower's collateral.

The function `claimETH` is risky to reentry attack. The function call triggers the `msg.sender.call{ value: ETHToSendToClaimer }("")` and state variable `_claims[msg.sender]` may be changed later. Since the real implementation of the external contract is unclear, and the address behind the interface is not clear, reentrancy is possible to take place.

The `getQuoteAtTick` function is a UniswapV3 function to calculate the number of ticks that can be exchanged under two cases, depending on the value comparison between `sqrtRatioX96` and `type(uint128).max`. The current implementation only covers the case of `sqrtRatioX96 <= type(uint128).max`. Reference: https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/OracleLibrary.sol#L49

Referring to [Ropsten, Rinkeby & Kiln Deprecation Announcement | Ethereum Foundation Blog](https://blog.ethereum.org/2022/06/21/testnet-deprecation), we noticed that there are chain IDs of Ropsten and Kiln testnets in the `setAddress()` function.

The function` getQueueStatus()` does not get the correct values. If a depositor calls function `removeFromQueue()`, variables `qLength`, `qHead`, and `qNext` are not updated. Note that this will not affect the actual queue of processing the swap in function `_processETH()`. However, if those values are used in the front-end, they might be inaccurate.

On L180, there is an unused field `decayedBaseRate` in struct `RedemptionTotals`. The relevant calculation of `decayedBaseRate` is in the `_calcDecayedBaseRate()` function of the contract Fees. The inconsistency is that when calculating the new base rate, the code implementation does not subtract the time by `1`. the decay factor is based on hours in the whitepaper, but in code implementation, it is based on minutes. In Math language: Whitepaper: $\ $ $b(t) := b(t-1) \times \delta^{dt}$ $\ $where $dt$ is in hours Code:$\ $ $\ $ $\ $ $\ $ $\ $ $\ $ $b(t) := b(t) \times \delta^{dt}$ $\ $ $\ $ $\ $ $\ $ $\ $where $dt$ is in minutes

There are two inconsistencies between the comments and implementations related to `truncateETH` And `truncatePAYD`: 1. In the function `_getETHGainFromSnapshots()`, the comment claims that `ETHGain` will round up to `1e6`. However, the implementation actually truncates `1e18` to `1e8`, since `truncateETH` is `1e10`. ```solidity // this would make individual figure more align to human readable form of say only 6 decimals instead of 18 if (ETHGain >= 0) { // truncate ETHGain -= ETHGain % int(truncateETH); } ``` 2. In the function `_getCompoundedStakeFromSnapshots()`, the comment claims that the truncation feature can be turned off by setting 0 to the `truncateXXX` variable, according to “// turn this off by setting truncatedPAYD to 0”. However, the state variables `truncatePAYD` and `truncateETH` are both declared as `immutable` with `1e10` as their values. ```solidity uint public immutable truncatePAYD = 1e10; // in the form of 10000000000(remove 10 digits) used by % uint public immutable truncateETH = 1e10; // in the form of 10000000000(remove 10 digits) used by % ```

There are some test cases that passed in the previous version but failed in the current version, `f05e7a2812aae28c1ddbbaa8db0c43ab27067906`. We understand that some test failures are because of the lack of mainnet deployment. Here we paste the results and failing test cases just for reference. ```bash ... ... 946 passing (13m) 3 pending 8 failing 1) Contract: Access Control: Liquity functions with the caller restricted to Liquity contract(s) FTXStaking setPriceFeedAddress(): success if address is contract: Error: Transaction reverted: function selector was not recognized and there's no fallback function at FTXStaking.<unrecognized-selector> (contracts/FTXStaking.sol:18) at FTXStaking.setPriceFeedAddress (contracts/FTXStaking.sol:183) at TruffleContract.setPriceFeedAddress (/Users/boxi.li/Downloads/stakedfinancing-coinspect-audit-2-f05e7a2812aa/node_modules/@nomiclabs/truffle-contract/lib/execute.js:169:26) at Context.<anonymous> (test/AccessControlTest.js:685:35) at processImmediate (internal/timers.js:464:21) 2) Contract: FETHToken Without proxy staking to ETH2 consensus layer - user's withdraw collateral, successfully unstakes with rewards: AssertionError: expected '1446575300000000' to equal '1446570000000000' + expected - actual -1446575300000000 +1446570000000000 at Context.<anonymous> (test/FETHToken_StakingTest.js:472:14) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5) 3) Contract: FTXPriceFeed FTXPriceFeed setup setAddresses should fail when called by nonOwner: AssertionError: expected 'VM Exception while processing transaction: reverted with reason string \'Ownable: caller is not the admin\'' to include 'Ownable: caller is not the owner' at assertRevert (utils/testHelpers.js:1341:16) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5) at Context.<anonymous> (test/FTXPriceFeedTest.js:22:7) 4) Contract: FTXPriceFeed FTXPriceFeed setup setAddresses should pass by owner(only work on forked mainnet): Error: VM Exception while processing transaction: reverted with reason string 'Account code size cannot be zero' at FTXPriceFeed.onlyAdmin (contracts/Dependencies/Ownable.sol:58) at FTXPriceFeed.checkContract (contracts/Dependencies/CheckContract.sol:11) at FTXPriceFeed.setAddresses (contracts/FTXPriceFeed.sol:106) at TruffleContract.setAddresses (/Users/boxi.li/Downloads/stakedfinancing-coinspect-audit-2-f05e7a2812aa/node_modules/@nomiclabs/truffle-contract/lib/execute.js:169:26) at Context.<anonymous> (test/FTXPriceFeedTest.js:30:26) at processImmediate (internal/timers.js:464:21) 5) Contract: FTXPriceFeed FTXPriceFeed setup setPrice should fail when on forked mainnet: AssertionError: expected 'VM Exception while processing transaction: reverted with reason string \'Ownable: caller is not the owner\'' to include 'FTXPriceFeed: cannot set price on mainnet' at assertRevert (utils/testHelpers.js:1341:16) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5) at Context.<anonymous> (test/FTXPriceFeedTest.js:49:7) 6) Contract: FTXPriceFeed FTXPriceFeed setup setPrice should pass on other network: Error: VM Exception while processing transaction: reverted with reason string 'Ownable: caller is not the owner' at FTXPriceFeed.onlyAdmin (contracts/Dependencies/Ownable.sol:58) at FTXPriceFeed.setPrice (contracts/FTXPriceFeed.sol:58) at TruffleContract.setPrice (/Users/boxi.li/Downloads/stakedfinancing-coinspect-audit-2-f05e7a2812aa/node_modules/@nomiclabs/truffle-contract/lib/execute.js:169:26) at Context.<anonymous> (test/FTXPriceFeedTest.js:56:26) at processImmediate (internal/timers.js:464:21) 7) Contract: FTXStaking unstake(): reverts if user tries to withdraw before staking period(test pass forked mainnet): AssertionError: expected 'expected true to be false' to include 'revert' at assertRevert (utils/testHelpers.js:1344:16) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5) at Context.<anonymous> (test/FTXStakingTest.js:153:5) 8) Contract: RewardsPool - Withdrawal of stability deposit - Reward calculations Stability Pool Withdrawal withdrawETHGain(): Pool-emptying liquidation increases epoch by one, resets scaleFactor to 0, and resets P to 1e18: AssertionError: expected 555001 to be at most 1000 + expected - actual -555001 +1000 at Context.<anonymous> (test/RewardsPool_SPWithdrawalToCDPTest.js:1058:14) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5)

There are identical implementations with different function names, including `openAllowed()`, `adjustAllowed()`, `redeemAllowed()`, etc.

There are several typos in the contracts, please see the above to find the locations, and the word with typos are listed here: - Accmulated - Collateralized - Cumulatives - Deafult - Pric - avilable - bootsrap - calculcate - collateralized - deactived - emited - excceeded - fufilled - implmentation - indentify - initializating - juse - legitmate - liqudated - minig - mutiples - neccessary - occured - ouput - oustanding - unforseen