Cyfrin / foundry-full-course-cu

GNU General Public License v3.0
3.41k stars 831 forks source link

Seem like we might have problem with getting the total collateral value in USD, if any of our user had deposited the two tokens that our protocol supports(weth, wbtc) #379

Closed EngrPips closed 1 year ago

EngrPips commented 1 year ago

Describe the enhancement

So when we were doing the conversion of user deposited collateral to USD we loop through each token supported by our contract and get the balance of the user for each of them and then went ahead to add it all up to the total collateral deposited , code below

function getTotalCollateralDepositedByUser(address _user) private view returns(uint256 _totalCollateralDeposited){
        for(uint256 i; i < s_protocolSupportedCollateralToken.length; i++){
            address[] memory copied_s_protocolSupportedCollateralToken = s_protocolSupportedCollateralToken;
            address token = copied_s_protocolSupportedCollateralToken[i];
            uint256 balanceOnToken = s_userToAmountDepositedOnVariousCollateralToken[_user][token];
            _totalCollateralDeposited += balanceOnToken;
        }

        return _totalCollateralDeposited;
    }

The above code seem very fine and okay , but lets assume the scenario where a user deposited 1 eth and 2 btc to our protocol, doing the above would help get the total deposited collateral but i think we would have problem when we want to convert the total collateral deposited to USD, the reason being which of the token USD price would we be using ?.

We never ran into that issue during the tutorial because we never deposited wbtc which makes everything turns out as expected.

So i think we are supposed to get the total collateral deposited by a user for each of the supported token store them separately and and then get the equivalent USD price for each of them separately and after getting the USD value for each of the deposited Collateral then we can sum them to have the total collateral deposited by each user in USD. like that we would be calculating the USD value for different token with their respective USD price.

Maybe i am not making sense but thats what i found and i think i should share as @PatrickAlphaC mentions the code would be going through an Audit. Thanks for all you do @PatrickAlphaC.

If this is confirmed to make sense then i would fork the repo and make the amendment, thanks 🙏

EngrPips commented 1 year ago

Look carefully at the code and saw that, this wasn't a problem it was just implemented in other way than i have thought of it.