# Foundry DeFi Stablecoin CodeHawks Audit Contest - Findings Report

# Table of contents
- ## [Contest Summary](#contest-summary)
- ## [Results Summary](#results-summary)
- ## High Risk Findings
- ### [H-01. `getTokenAmountFromUsd()` doesn't consider different token and oracle decimals](#H-01)
- ### [H-02. Ensure `DSCEngine` is the owner of `DecentralizedStableCoin`](#H-02)
- ## Medium Risk Findings
- ### [M-01. Chainlink staleness threshold should be set per asset](#M-01)
- ### [M-02. Precision loss may occur when calculating health factor](#M-02)
- ### [M-03. Use `safeTransfer` for token transfer](#M-03)

# <a id='contest-summary'></a>Contest Summary

### Sponsor: Cyfrin

### Dates: Jul 24th, 2023 - Aug 5th, 2023

[See more contest details here](https://www.codehawks.com/contests/cljx3b9390009liqwuedkn0m0)

# <a id='results-summary'></a>Results Summary

### Number of findings:
- High: 2
- Medium: 3
- Low: 0

# High Risk Findings

## <a id='H-01'></a>H-01. `getTokenAmountFromUsd()` doesn't consider different token and oracle decimals

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L347

## Summary
`getTokenAmountFromUSD()` returns an incorrect amount for tokens with decimals != 18. It also returns an incorrect amount if chainlink oracle uses a different decimal than 8.

## Vulnerability Details
`getTokenAmountFromUSD()` calculates the final amount as follows:

return ((usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION));

This is fine for current collaterals: WETH and WBTC. However, if new collateral is added in future with different decimals, the protocol will return incorrect amount to liquidator. Here is the derivation of correct formula:
- Let `C` be chainlink oracle decimal.
- Let `D` be token decimal.
- `price/1eC` USD gets 1eD tokens.
- `(price/1eC) * (usdAmountInWei/1e18)` USD gets `1eD * (usdAmountInWei/1e18)` tokens.
- `(usdAmountInWei/1e18)` USD gets:

`usdAmountInWei * 1eD * 1eC / (price * 1e18)` which is the final formula to be used in `getTokenAmountFromUsd()`.

`C` and `D` can be fetched by calling `decimals()` on the collateral token and chainlink oracle. To save gas, you can also store these values permanently.

## Impact
For tokens and chainlink oracle deviating from the 18 and 8 decimals respectively, protocol will work incorrectly.

## Tools Used
Manual review.

## Recommendations
Update the return value of [`getTokenAmountFromUsd()`](https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L347) to consider token decimals and chainlink decimals.
## <a id='H-02'></a>H-02. Ensure `DSCEngine` is the owner of `DecentralizedStableCoin`

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L112

## Summary
Any address can be passed as `dscAddress` to `DSCEngine`'s constructor. So there is no guarantee that `dscAddress` is the intended stablecoin, or if `DSCEngine` is the owner of `dscAddress`. In both these cases, the protocol won't work.

## Vulnerability Details
If `DSCEngine` is not the owner of `dscAddress`, then the mint and burn calls will revert as they are guarded by `onlyOwner` modifier.

## Impact
Protocol will break, and no one can mint or burn DSC tokens.

## Tools Used
Manual review.

## Recommendations
Update `DSCEngine's` constructor to deploy `DSCEngine`. This ensures that the correct contract is deployed at `dscAddress` and the ownership is also set correctly:

constructor(address[] memory tokenAddresses, address[] memory priceFeedAddresses) {
        // USD Price Feeds
        if (tokenAddresses.length != priceFeedAddresses.length) {
            revert DSCEngine__TokenAddressesAndPriceFeedAddressesMustBeSameLength();
        }
        // For example ETH / USD, BTC / USD, MKR / USD, etc
        for (uint256 i = 0; i < tokenAddresses.length; i++) {
            s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i];
            s_collateralTokens.push(tokenAddresses[i]);
        }
        i_dsc = new DecentralizedStableCoin();
}

# Medium Risk Findings

## <a id='M-01'></a>M-01. Chainlink staleness threshold should be set per asset

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/libraries/OracleLib.sol#L19

## Summary
Every chainlink oracle comes with its own `heartbeat` which denotes the maximum time gap between 2 price updates. Chainlink price feeds can go stale, so the stablecoin protocol has a staleness check but the threshold is set globally and doesn't account for different oracles.

## Vulnerability Details
This protocol sets the staleness check threshold to 3 hours:

uint256 private constant TIMEOUT = 3 hours; // 3 * 60 * 60 = 10800 seconds

However, for some assets this can be too long or too short. For example [ETH/USD oracle](https://data.chain.link/ethereum/mainnet/crypto-usd/eth-usd) has a heartbeat of 1 hour, and stablecoin oracles usually have a heartbeat of 24 hours.

## Impact
Liquidation may occur on stale prices harming the protocol and its users.

## Tools Used
Manual review.

## Recommendations
Create a mapping to store a timeout for each collateral asset. At the time of fetching price for an asset, use the associated timeout for staleness check.
## <a id='M-02'></a>M-02. Precision loss may occur when calculating health factor

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L330

## Summary
`collateralAdjustedForThreshold` calculation can lose precision when `collateralValueInUsd * LIQUIDATION_THRESHOLD` is not exactly divisible by `LIQUIDATION_PRECISION`. The resulting value would roundoff to the lowest value and would lead to precision loss since decimals are not handled in Solidity.

## Vulnerability Details

uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;

For example, for `collateralValueInUsd = 267`, `collateralAdjustedForThreshold = 133` when the actual value should be `133.5`.

## Impact
Calculated health factor will be lower than the real value.

## Tools Used
Manual review

## Recommendations
We multiply `1e18` with `collateralAdjustedForThreshold`. Instead, first multiply and calculate the entire numerator before dividing:

+return (collateralValueInUsd * LIQUIDATION_THRESHOLD * 1e18) / (totalDscMinted * LIQUIDATION_PRECISION);
-uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
-return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;

## <a id='M-03'></a>M-03. Use `safeTransfer` for token transfer

### Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L157

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L274

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L287

## Summary
If non-compliant ERC20 tokens are added in future as collateral (like USDT), then protocol may not be able to handle the transfers.

## Vulnerability Details
The protocol use `transfer` and `transferFrom` to transfer collateral token. It also verifies that its return value is `true`:

bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
    revert DSCEngine__TransferFailed();
}

For WETH and WBTC, if transfer doesn't revert, `success` is always true. Hence this check can be skipped to save gas. However, if we want to add other collateral tokens in future, it's recommended to use OpenZeppelin's `SafeERC20` library which accommodates non-compliant ERC20 tokens. For example, USDT which doesn't return anything on a successful transfer.

## Impact

## Tools Used
Manual review.

## Recommendations
Use OpenZeppelin's [`SafeERC20`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L36-L44) library to handle ERC20 transfer. Add the following at the top of the contract:

using SafeERC20 for IERC20;

Now replace `.transfer` and `.transferFrom` with `.safeTransfer` and `.safeTransferFrom`. For instance:

+IERC20(tokenCollateralAddress).safeTransferFrom(msg.sender, address(this), amountCollateral);
-bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
-if (!success) {
-    revert DSCEngine__TransferFailed();
-}