0x52 - Swapping 100 tokens in DepositReceipt_ETH and DepositReciept_USDC breaks usage of WBTC LP and other high value tokens

[github-actions]

0x52

high

Swapping 100 tokens in DepositReceipt_ETH and DepositReciept_USDC breaks usage of WBTC LP and other high value tokens

Summary

DepositReceipt_ETH and DepositReciept_USDC checks the value of liquidity by swapping 100 tokens through the swap router. WBTC is a good example of a token that will likely never work as LP due to the massive value of swapping 100 WBTC. This makes DepositReceipt_ETH and DepositReciept_USDC revert during slippage checks after calculating amount out. As of the time of writing this, WETH also experiences a 11% slippage when trading 100 tokens. Since DepositReceipt_ETH only supports 18 decimal tokens, WETH/USDC would have to use DepositReciept_USDC, resulting in WETH/USDC being incompatible. The fluctuating liquidity could also make this a big issue as well. If liquidity reduces after deposits are made, user deposits could be permanently trapped.

Vulnerability Detail

    //check swap value of 100tokens to USDC to protect against flash loan attacks
    uint256 amountOut; //amount received by trade
    bool stablePool; //if the traded pool is stable or volatile.
    (amountOut, stablePool) = router.getAmountOut(HUNDRED_TOKENS, token1, USDC);

The above lines try to swap 100 tokens from token1 to USDC. In the case of WBTC 100 tokens is a monstrous amount to swap. Given the low liquidity on the network, it simply won't function due to slippage requirements.

function _priceCollateral(IDepositReceipt depositReceipt, uint256 _NFTId) internal view returns(uint256){  
    uint256 pooledTokens = depositReceipt.pooledTokens(_NFTId);      
    return( depositReceipt.priceLiquidity(pooledTokens));
}

function totalCollateralValue(address _collateralAddress, address _owner) public view returns(uint256){
    NFTids memory userNFTs = loanNFTids[_collateralAddress][_owner];
    IDepositReceipt depositReceipt = IDepositReceipt(_collateralAddress);
    //slither-disable-next-line uninitialized-local-variables
    uint256 totalPooledTokens;
    for(uint256 i =0; i < NFT_LIMIT; i++){
        //check if each slot contains an NFT
        if (userNFTs.ids[i] != 0){
            totalPooledTokens += depositReceipt.pooledTokens(userNFTs.ids[i]);
        }
    }
    return(depositReceipt.priceLiquidity(totalPooledTokens));
}

One of the two functions above are used to price LP for every vault action on Vault_Velo. If liquidity is sufficient when user deposits but then drys up after, the users deposit would be permanently trapped in the in the vault. In addition to this liquidation would also become impossible causing the protocol to assume bad debt.

This could also be exploited by a malicious user. First they deposit a large amount of collateral into the Velodrome WBTC/USDC pair. They take a portion of their LP and take a loan against it. Now they withdraw the rest of their LP. Since there is no longer enough liquidity to swap 100 tokens with 5% slippage, they are now safe from liquidation, allowing a risk free loan.

Impact

LPs that contain high value tokens will be unusable at best and freeze user funds or be abused at the worst case

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_ETH.sol#L93-L152

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_USDC.sol#L75-L130

Tool used

Manual Review

Recommendation

Change the number of tokens to an immutable, so that it can be set individually for each token. Optionally you can add checks (shown below) to make sure that the number of tokens being swapped will result in at least some minimum value of USDC is received. Similar changes should be made for DepositReceipt_ETH:

constructor(string memory _name, 
            string memory _symbol, 
            address _router, 
            address _token0,
            address _token1,
            uint256 _tokensToSwap,
            bool _stable,
            address _priceFeed) 
            ERC721(_name, _symbol){

    ...

    if (keccak256(token0Symbol) == keccak256(USDCSymbol)){
        require( IERC20Metadata(_token1).decimals() == 18, "Token does not have 18dp");

+       (amountOut,) = _router.getAmountOut(_tokensToSwap, token1, USDC);

+       //swapping tokens must yield at least 100 USDC
+       require( amountOut >= 1e8);
+       tokensToSwap = _tokensToSwap;
    }
    else
    {   
        bytes memory token1Symbol = abi.encodePacked(IERC20Metadata(_token1).symbol());
        require( keccak256(token1Symbol) == keccak256(USDCSymbol), "One token must be USDC");
        require( IERC20Metadata(_token0).decimals() == 18, "Token does not have 18dp");
        
+       (amountOut, ) = _router.getAmountOut(_tokensToSwap, token0, USDC);

+       //swapping tokens must yield at least 100 USDC
+       require( amountOut >= 1e8);
+       tokensToSwap = _tokensToSwap;
    }

[kree-dotcom]

Fixed kree-dotcom/Velo-Deposit-Tokens@76bb63d

Both DepositReceipts have had the swap quantity changed to an immutable. DepositReceipt_USDC contains further checks to ensure the swap quantity would receive between 100 and 105 USDC on deployment so we know it is the right scale. This check could also be done with DepositReceipt_ETH but because the value of ETH is dynamic it would be a little more complex so instead we just included a non-zero value check.

Then corrected a mistake with still using the HUNDRED constant for scaling this trade in this commit
kree-dotcom/Velo-Deposit-Tokens@d8e7651

[IAm0x52]

Fixes looks good. Now uses a variable swap amount instead of always using 100 full tokens