search

hats-finance / VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f

LP token lending protocol
MIT License
2 stars 1 forks source link

The check for available liquidity is incorrect if the token was ever lent #58

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @GalloDaSballo Submission hash (on-chain): 0x39e6bdd72c6d292861d39347759d05434e7dfa37f4bbf1c8f6089f093fe6de87 Severity: low severity

Description: Description\

https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/f14637deda71be440d1cfbf31cc12ede69ff3a6b/packages/contracts/contracts/protocol/lendingpool/LendingPoolCollateralManager.sol#L184-L187

        if (!receiveAToken) {
            uint256 currentAvailableCollateral = IERC20(vars.collateralAsset)
                .balanceOf(vars.collateralAToken);
            currentAvailableCollateral = currentAvailableCollateral.add(IAToken(vars.collateralAToken).getStakedAmount()); // Looks off

Looks wrong cause totalSupply is cognizant of the index vs staked doesn't grow over time

Can be verified by checking the totalSupply on any live contract such as the CRV Market on Mainnet https://etherscan.io/address/0x8dAE6Cb04688C62d939ed9B68d32Bc62e49970b1

Which over a few seconds grows, e.g.

298530189024582799736699096 uint256 298530189772748324764861196 uint256

This will make it so that the contract will return

Attack Scenario\

This creates a scenario where, if enough tokens are borrowed and the totalSupply has grown sufficiently (a bunch of interest yet to be paid), the check will pass, but liquidations will not work as intended as the AToken will not have sufficient tokens to perform the liquidation

The TX should revert at Atoken.burn due to lack of liquidity https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/f14637deda71be440d1cfbf31cc12ede69ff3a6b/packages/contracts/contracts/protocol/tokenization/AToken.sol#L165-L166

        IERC20(_underlyingAsset).safeTransfer(receiverOfUnderlying, amount);
ksyao2002 commented 1 year ago

Again, tokens with staking rewards will not be lent out https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/issues/33 https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/issues/32. With the two cases

  1. Liquidated token is also borrowable (this means that totalStakedAmount is zero so it doesn't affect the calculations)
  2. Liquidated token is not borrowable (this means that index never grows so the attack scenario doesn't apply)

Please let me know if there is anything I am missing.

GalloDaSballo commented 1 year ago

What I would add is that for reasons similar to #57 if a token started as borrowable, and then a staking contract is added, a migration plan of some sort will need to be enacted, as otherwise the older tranches will run into this bug + the one from #57

While newer pools will have the aToken index to 1-1 and for that reason all checks will pass

ksyao2002 commented 1 year ago

That's a good consideration. I don't think we would have any tokens that we allow for borrowing, and then enable staking rewards for, since borrowing is only for the base tokens (like USDC) that don't have staking systems, and staking systems are generally for lp tokens that we won't ever consider for borrowing.

0xcuriousapple commented 1 year ago

hmm, there are additional issues with allowing lp tokens as borrowable. good to know that you are not looking to make them borrowable. the whole accounting for current rewards distribution doesn't work once it's borrowable, one could easily game it by doing a borrow and deposit of the lp tokens in loop

ksyao2002 commented 1 year ago

Yeah, we coded it with the intent that they are not borrowable. It drastically changes accounting logic if they are borrowable.