**FXD protocol fixes and commits with description
as of Feb 22nd Wednesday 2023**

# **Audit fix doc**

# **Part 1**

branch : auditFeb

Please refer to commit history in the link below
https://github.com/Into-the-Fathom/fathom-stablecoin-smart-contracts/commits/auditFeb

adding commits from bae33c7d49f76317ac5fa832a5f9961c842ce66c
which was the one submitted for audit initially.

fix #1
precision loss from rounding up
this issue was not included in the audit report, however, it is an important for liquidation process, therefore, fixed in the first place.

Commit hash : 3c1394d76b9f4aada4836e670a726d69adf5432f

Problem : During liquidation process, numbers became inaccurate during cross contract calls

Goal : Fixing rounding issues for accurate liquidation

Description : Liquidation engine calls deposit fn to stablecoinAdapter.
Originally deposit fn in stablecoinAdapter multiplies info.actualDebtValueToBeLiquidated by RAY(10**27) and causes rounding up issue which hurts accounting accuracy. For accurate accounting, stablecoinAdapter should accept number in RAD directly from liquidation engine.

fix #2

Commit hash :
47b276731d39406e57c63f1a2177a84b754f7424

Problem : Anyone can directly interact with AnkrCollateralAdapter to stake XDCs to liquidStaking protocol.

Goal : Adding access control to AnkrColAdapter so that liquidStaking can be accessible only through Fathom protocol.

Description :
Two changes were made to ensure FATHOM to be the only entry point to liquidStaking.

1) adding access control to AnkrColAdapter so that only whitelisted addresses or proxyWallets created from Fathom protocol can call certain fns in AnkrColAdapter

2) proxyWallet used have address of proxyActions library fns to be included in the first argument of execute funtion. From this change, proxyWallet will always have a fixed proxyActions contract addresse predefined and stored in proxyActions Storage. execute function in proxyWallet now only has _data bytes memory as the only argument

fix #3

Commit hash :
fcd0c892c30785fc744027992738b80cef9ddf75

Problem : When some of the iterations within batch liquidation fn in liquidation engine fails, it is impossible to know the reason, therefore, no one is able to adjust fn calls based on failure.

Goal : Make the failing iteration and its reason be known.

Description : We added an event called LiquidationFail so that the reason string will be emitted with the event.

fix #4

commit hash :
5de9d262157bcc6fa2d5175e098b47be6315abeb

issue : 2.1.2 certsToMoveRatio not calculated properly in AnkrCollateralAdapter

from the audit report 1st iteration

Goal : make sure that certsToMoveRatio is calculated in all cases

Description :
certsToMoveRatio will be WAD when *share >= stake[source]* and in other cases will be calculated as before. (as recommended)
We also added a line of verification to ensure that moveCerts fn will be called when
stake[_source] != 0.

fix #5

commit hash :

bdc598e35947f1c2a98422dcb03d273505814278

issue : 2.2.4 There is no validation of return data in
FathomSwapLibrary

from the audit report 1st iteration

Goal : to make sure that results and data from cross contract calls are validated.

Description:
To sum up, validations of the returned datas are added(as recommended)

After experimenting getPair static call's different returned data, verifications of returned data is introduced for audit fix

* 0)Within getReserves function routine, getPair static call's returned bool data is checked to be true so that failing static call would halt the function's execution
* 1)make sure that the returned data is not empty(in case contract called was not made successfully although returned bool data show true)
* 2)make sure that the pair address is not a zero address(A zero address in returned data is the case when static call was successfull but there was no pair made in the DEX's factory)

Once the returned data is checked as valid, it will be used as pair address to where 'getReserves()' staticCall will be called. There are two verifications of returned data after that.

* 0)returned bool from the staticCall needs to be true
* 1)returned data's length should be same as 0x60

fix#6

commit hash :
c4089292a31545515b31ba736c0f6d86c6441987

issues : all priceFeed related ones

* 2.1.1 Current price is returned in the feed in DelayFathomOraclePriceFeed

* 2.1.3 It is possible for an old price to become valid in price feeds

* 2.2.1 Incorrect validation of priceLife in SimplePriceFeed

* 2.2.2 There is no validation in DelayFathomOraclePriceFeed - fixed by adding validation for zero address

* 2.2.3 Price does not get updated after unpause in price feeds - fixed. Now we added _peekPrice call to update the price if contract was unpaused
* 2.2.7 Too big _lastUpdate will block price changing for a long time - now we don't expect prices from the future, we added validation to revert if lastUpdate is greater than the current block.timestamp

* 2.3.3 Time delay can be higher than price life in DelayFathomOraclePriceFeed

* 2.3.4 Unsafe initialize of accessControl in DelayFathomOraclePriceFeed - fixed issue by adding validation for zero addres

* 2.3.5 fathomOracle , token0 and token1 cannot be changed in DelayFathomOraclePriceFeed

from the audit report 1st iteration

descriptions:

SimplePriceFeed is now outside of scope. Therefore, SimplePriceFeed is moved to mocks contract folder since it will be not be used neither in production nor demo.

2.1.1 Current price is returned in the feed in DelayFathomOraclePriceFeed

To sum up the changes, it is made in a way that real delayed price will be used(as recommended).

* in line 11, 12 of DelayFathomOraclePriceFeed.sol : Introduced new variable "delayedPrice" to store delayed price and return it on readPrice and peekPrice calls
* in line 53 of DelayFathomOraclePriceFeed.sol : Now readPrice function returns delayed price instead of current price
* in line 115 of DelayFathomOraclePriceFeed.sol : Now peek price return delayed price instead of current one
* in line 122 of DelayFathomOraclePriceFeed.sol : On price update we will save the latest saved price(which will be as old as we set in time delay) as delyedPrice. this price will be returned on readPrice and peekPrice calls.
* in line 128 of DelayFathomOraclePriceFeed.sol : Here we will save the current price from oracle as the latestPrice

2.1.3 It is possible for an old price to become valid in
price feeds

* in line 78 of DelayFathomOraclePriceFeed.sol : Added peekPrice call to the setTimeDelay function. Now when we set new timeDelay - price will be updated according to new config (as recommended).

2.2.1 Incorrect validation of priceLife in SimplePriceFeed

* SimplePriceFeed is taken out of the scope. No longer it will be used for neither demo nor production.

2.2.2 There is no validation in DelayFathomOraclePriceFeed

* in line 29, 30 of DelayFathomOraclePriceFeed.sol : fixed by adding validation for zero address as recommended.
* in line 31 of DelayFathomOraclePriceFeed.sol : sorting tokens procedure is added as recommended.

2.2.3 Price does not get updated after unpause in price feeds - fixed. Now we added _peekPrice call to update the price if contract was unpaused

* in line 108 of DelayFathomOraclePriceFeed.sol : Now we added peekPrice call to update the price if contract was unpaused as recommended.

2.2.7 Too big lastUpdate will block price changing for a long time

* in line 123 to 126 of DelayFathomOraclePriceFeed.sol : Now we don't expect prices from the future, we added validation to revert if lastUpdate is greater than the current block.timestamp.

2.3.3 Time delay can be higher than price life in
DelayFathomOraclePriceFeed

* in line 71 of DelayFathomOraclePriceFeed.sol : fixed by adding validation that checks if the price life we want to set is not less then timeDelay.

* in line 77 of DelayFathomOraclePriceFeed.sol : fixed by adding validation that checks if timeDelay we set in not bigger then price life.

2.3.4 Unsafe initialize of accessControl in DelayFathomOraclePriceFeed

* in line 25 of DelayFathomOraclePriceFeed.sol : fixed issue by adding validation for zero addres as recommended.

2.3.5 fathomOracle , token0 and token1 cannot be changed in DelayFathomOraclePriceFeed

In summary, setters are added for fathomOracle, token0 and token1 as recommended

* in line 83 and 90 of DelayFathomOraclePriceFeed.sol : fixed by adding a setter, now we can set the new token0 and token1 in case we fail to pass correct parameters during the contracts deploy, and no need to redeploy price feed

* in line 97 of DelayFathomOraclePriceFeed.sol : fixed by adding a setter, now we can set the new oracle in case we fail to pass correct parameters during the contracts deploy, and no need to redeploy price feed

# **Part 2**

branch : auditFeb

Please refer to commit history in the link below
https://github.com/Into-the-Fathom/fathom-stablecoin-smart-contracts/commits/auditFeb

fix#7

commit hash :
e1306ed55c704c496d4e03d1a1c71d19a5e0eac2

adding blackListing to ankrColAdapter

* adding removal from whitelist should have been implemented along with whitelisting feature in AnkrCollateralAdapter

* adding require statment to whiteList fns so zero address cannot be whiteListed

fix#8

DexPriceOracle fixes

commit hash :
a6736a96dfeb939e0cce01ddc578859b2ccfb428

issues : from first interation
2.2.6 Small denominator in DexPriceOracle
2.3.6 Incorrect price with equal tokens in
DexPriceOracle

2.2.6 Small denominator in DexPriceOracle

* in line 31 to 33 of DexPriceOracle.sol : Casting to the same decimals before price calculation. If decimals1 is higher than decimals0 the code adds more zeros to r0. It makes both reserves has the same decimals (bigger of both) which enables priceCalculation with division.

2.3.6 Incorrect price with equal tokens in
DexPriceOracle

* in line 25 of DexPriceOracle.sol : fixed by reverting the same tokens

fix#9

commit hash :
5fcd46365e827a02ab92454ca83cb30f3d72b0ac

issue : from first iteration

2.2.5 It is not possible to cage already caged contracts in ShowStopper

* in line 209 of BookKeeper.sol
* in line 244 of LiquidationEngine.sol
* in line 101 of PriceOracle.sol
* in line 102 of SystemDebtEngine.sol
* in line 48 of SystemDebtEngine.sol
* in line 62 of TokenAdapter.sol

# **Part 3**

fix#10
commit hash : a8cfb441ce277497e9a3c74d03e501678237dddb

issue : stableSwapModule related
from
fix#10 is stableSwapModule

2.1.1 Incorrect calculation in the depositTokens function of the StableSwapModule contract

* in line 140 to 145 of StableSwapModule.sol, fixed the issue

2.1.2 Incorrect calculation in the swapTokenToStablecoin function of the StableSwapModule contract

* in line 105 of StableSwapModule.sol, fixed the issue with _convertDecimals internal function

2.1.3 Incorrect calculation in the swapStablecoinToToken function of the StableSwapModule contract

* in line 125 of StableSwapModule.sol, fixed the issue by doing all calculation in 18 decimals, and converting to native token decimals

uint256 tokenAmount = _convertDecimals(_amount - fee, 18, IToken(token).decimals());

2.1.4 dailySwapLimit calculations only work for normal tokens in the StableSwapModule contract

* in line 110 of StableSwapModule.sol
_udpateAndCheckDailyLimit(tokenAmount18);
tokenAmount18 is in 18 decimal

* in line 129 of StableSwapModule.sol _udpateAndCheckDailyLimit(_amount);
_amount is in 18 decimal

Therefore, dailySwapLimit calculation is now good.

2.1.5 withdrawFees only works for normal tokens in the StableSwapModule contract

In summary: FXDFee is accounted in FXD's decimal and TokenFee is accounted in Token's decimal, therefore, it is okay.

There are some changes we had to make due to issues found when the front-end was tested. The detail is in the lines below.

There are two changes made in StableSwapModule that is related to fees.

**First, fn swapStablecoinToToken's argument will now take amount of FXD(FathomStablecoin) to be paid. And the fee will be taken and accounted in Token.**
**Second, because now there is new TokenFee, withdraw functions will not only withdraw FXD but also Token.**

The first change was implemented due to issues on front-end number and onchain number difference.

Before, fn swapStablecoinToToken would take argument of amount as TokenAmount that user will receive rather than Stablecoin amount that user is willing to swap to Token. And the fee was collected in FXD. This is a terrible UX for user since in fn SwapStablecoinToToken user needs to think how much to Token to receive and fn swapTokenToStablecoin, user needs to think how much Token to pay.

On the front-end side, to get rid of this confusion in fn swapStablecoinToToken flow, so that user can just think about how much token|FXD to pay, we added some calculations so that user can specify in an input box how much Stablecoin he/she wants to pay and then calculation how much Tokens will be received.

Although the above fix got rid of the UX confusion, there was slight differene between stablecoin that was paid onchain in fn swapStablecoinToToken and what the users sees on the front-end.

Therefore, first change mentioned above was implemented.

Now let's talk about second change. For Stablecoin withdrawl, due to the fact that now Token fee is collected as well, withdrawl fn should not only withdraw FXD but also Token. And the accounting of FXDFee and TokenFee are done with its own decimals.

*in line 34,35 of StableSwapModule.sol FXDFee and TokenFee are introduced
*in line 114 FXDFee is collected
*in line 113 TokenFee is collected, the tokenFee accounting will be done in its decimal so that withdrawl will happen smoothly without decimal conversion

# **Part 4**

fix#11

2.1.6 Incorrect use of to18ConversionFactor variable in AnkrCollateralAdapter

commit hash:
a4bea3aa7be37d72b94be48b5f27c1be4a8d8d3c

* in line 192 of AnkrCollateralAdapter.sol, needless to18ConversionFactor multiplication is removed. aXDCc will always be 18 decimals and this collateralAdapter will only deal with aXDCc.

* converTo18 method is not needed. IAnkrColAdapter replaced IFarmableTokenAdapter.sol

and the
delete simplePriceFeed init
just cherry pick and that's it.

fix#12

commit hash:
f81313189550472b8e586a5480004df0e7172d78

issue: tryCatch error handling is not sufficient, pointed out by one of the auditors in the tech support chat.

and

2.3.17 Array lengths are not validated in
LiquidationEngine
SEVERITY WARNING
STATUS NEW
SEVERITY WARNING
audit fix
from 3rd audit report iteration

* in line 110 of LiquidationEngine, array length validation is added

* in line 123 of LiquidationEngine, more error cathing is added

fix#13

commit hash:
c4089292a31545515b31ba736c0f6d86c6441987

auditor's advice on telegram chat

https://github.com/Into-the-Fathom/fathom-stablecoin-smart-contracts/blob/c4089292a31545515b31ba736c0f6d86c6441987/contracts/main/price-feeders/DelayFathomOraclePriceFeed.sol#L66

hasRole will return a bool, need to wrap it with a requirement

https://github.com/Into-the-Fathom/fathom-stablecoin-smart-contracts/pull/112

fix#14

TWAP implementation for priceOracle

commit hash:
71b96efde96091f1a8909c52141133a113ff9bda

fix#1??

also auditor's advice on ankrColAdaptor's whiteListing functionality

change ankrColAdaptor's whiteListed address's role to AccessControlConfig's new role

also liq. engine's whiteListing as

AccessControlConfig's new role

requirements,

well, the functions protected by whiteListed by proxywallet only should now be protected by accessControl and proxyWallet only modifier

scripts that need to be changed because of change above

/Users/sangjun.lee/Sangjun/fathom-stablecoin-smart-contracts/scripts/deployment/5_liquidation-engine/config/config_liquidation-engine-whiteListBot.js

instead of liquidationEngine.whitelist, better use grantRole, for this I need to add a liquidator role

/Users/sangjun.lee/Sangjun/fathom-stablecoin-smart-contracts/scripts/migrations/deployment/4_add-roles.js

line 51,52,53 ankrCollateralAdaptor.whitelist
need to change to grantRole
hm... better come up with a good name for ankrCollateralAdaptor's whitelist

/Users/sangjun.lee/Sangjun/fathom-stablecoin-smart-contracts/scripts/tests/unit/stablecoin-core/LiquidationEngine.test.js
43,26: await liquidationEngine.whitelist(DeployerAddress);
44,26: await liquidationEngine.whitelist(AliceAddress);

liquidation engine.whitelist needs to be changed to grantRole