Summary

During the audit review of the DSCEngine.sol contract, I found multiple for loops which are not as gas efficient as they could be. This can be fixed by creating local variables which store the lengths of the tokenAddresses.length and s_collateralTokens.length arrays also using unchecked would reduce gas cost as well.

Vulnerability Details

In DSCEngine.sol:118 and line 353. Both functions have a for loop which every time it loops it calls the length of a given array, tokenAddresses.length and s_collateralTokens.length. This means will have multiple SLOADs instead of having one SLOAD for the whole array. By not using unchecked you are costing the contract more gas as without the code will check for underflow/overflow error cases every time the loop runs. You do not need to spend the extra gas checking for underflow/overflow error cases as the for loop conditional statements stop the variable i from overflow because of the condition i < length, where length is defined as uint256. The maximum value i can reach is max(uint)-1. Thus, incrementing i inside unchecked block is safe and consumes lesser gas.

Impact

The impact is the DSCEngine.sol contract costs more gas to deploy and the function getAccountCollateralValue cost more to execute. The getAccountCollateralValue function will have to be called more than once as it is needed to get the value of the collateral and if the function costs more gas then this will make the contract more gas expensive to use and worsen the user experience as each user will have to pay more gas to run the contract.

Tools Used

Foundry

Recommendations

Create a local variable for each for loop which stores tokenAddresses.length and s_collateralTokens.length: uint length = tokenAddresses.length and uint length = s_collateralTokens.length This way the for loop can call this variable as one SLOAD and save gas instead of having to read the length of the array every loop which would cost more gas and require multiple SLOADs. Put the code of both for loops inside of an unchecked brackets: unchecked{ s_priceFeeds[tokenAddresses[i]] = priceFeedAddresses[i]; s_collateralTokens.push(tokenAddresses[i]); }
unchecked{ address token = s_collateralTokens[i]; uint256 amount = s_collateralDeposited[user][token]; totalCollateralValueInUsd += getUsdValue(token, amount); }