Contract not able to process withdraws or deposit

kingjacob commented 4 years ago

Description

On attempting to withdraw the contract threw an error (photo below), and then when I sent the transaction [link to] it failed with a 'Fail with error 'redeemTokens zero'' error.

Attempted to deposit as well and that transaction failed as well [Etherscan link]

[edit: the deposit fail is likely a seperate gas limit too low issue #57 cause I was able to get a deposit through? [link] ]

Steps to reproduce

Deposit Dai into the contract at metamoneymarket.com
Attempt to withdraw Dai via the metamoneymarket ui

Which network did you use?

Mainnet

Supporting info

The contract is showing the correct supply for the address.

maxweng commented 4 years ago

I think the issue is that there was a small amount of CTokens left in the CompoundAdapter when the user withdrew his assets, in this case it was 208980846 left. And it was so small that made Compound failed to return redeemTokens greater than 0 from the divScalarByExpTruncate() function on line #855 in CToken.sol, which caused the revert of comptroller.redeemVerify() on line #917 of CToken.sol.

We probably need to set a minimum underlying token amount to redeem from the adapters when users try to withdraw.

The whole failed call stack can be seen here: https://bloxy.info/tx/0x1bb73adba6398ccaed390f0c0f8362be621d8ef5c51c96012244b61d5a073470

kingjacob commented 4 years ago

Depositing 1 Dai when compound was the highest rate resolved the issue in the deployed smart contract, and was able to withdraw. More permanent fix still needed.

fvictorio commented 4 years ago

I think @maxweng is right. The problem seems to be that the Compound adapter has only 1 cDAI (in wei) in its balance, and there seems to be an issue in Compound when you redeem exactly the amount of underlying equivalent to 1 cDAI. I tested this a lot of ways, so I'm pretty sure that 's the case. If I'm right, then some solutions are:

Try to avoid any scenario where the amount of cTokens left in the adapter is 1 (again, in wei). This is almost for sure a rounding issue. This might be the right solution, but it will take some time to check all the math. Also, if for some reason the scenario repeats, the bug is still there.
Check if this is the case, and handle it as a special case. I don't like this option because it would mean adding an ugly if in the contract just for this.
Manual fix: send some cDais to the adapter when this happens. I just did this (now it has 2). This seems to fix it for now.

maxweng commented 4 years ago

I think the issue is related to how Compound calculates the amount of CToken being redeemed. If you track down to the code on line #855 in CToken.sol, it calls divScalarByExpTruncate(), which calls truncate(). And what it does is basically to divide user's withdrawal amount by 1e18. So any withdrawal amount less than 1e18 wei is gonna return 0 redeemable CToken, which results to the failed withdrawing in Compound.

My proposed quick fix here would be, on line #135 of MetaMoneyMarket.sol, to change the checking condition for supply from if (supply == 0) to if (supply >= 1e18).

But a better one maybe to add a new function like getMinWithdraw() in IMoneyMarketAdapter interface to adapt to different lending protocol requirements.

maxweng commented 4 years ago

Also suggest to change the title of this issue to make it more clear.

fvictorio commented 4 years ago

Hi @maxweng. I'm not really sure about the >= 1e18 fix. I manually redeemed amounts smaller than that from Compound and it worked fine. Plus, this fix would mean that people have to invest at least 1.0 of each token. This is not a lot for DAI, but it is for WBTC (actually, it's worse, because WBTC has 8 decimals, so you would need 1e10 BTC to deposit). Am I missing something here?

An alternative fix I can think of is in the CompoundAdapter. Basically, when totalSupply is called and before checking the balanceOfUnderlying, we check the plain balanceOf. If the adapter has a single wei of that cToken, it just returns 0. This, I think, would fix the issue in the MetaMoneyMarket contract, because that adapter would just be ignored.

pablofullana commented 4 years ago

@kingjacob @maxweng any feedback on @fvictorio's proposal?

primeradiant / MetaMoneyMarket