Liquidation doesn't work as intended in recovery mode about dev HOT 13 CLOSED

This is a bug in checkTCRAndSetRecoveryMode.

https://github.com/cvalkan/cleverage/blob/c059b444f05170c223ea98ed98fc3f7ac83462f8/packages/contracts/contracts/CDPManager.sol#L1036-L1045

When the state variable recoveryMode is already true in the contract and TCR is below 150%, neither branch of the conditional is evaluated, therefore the local variable recoveryModeInMem is never initialized, and the default bool value of false is returned.

from dev.

Hmm, one more thing I noticed. checkTCRAndSetRecoveryMode should either be made internal or its price parameter should be removed. As it stands now, anyone can set recoveryMode to an incorrect value by calling it with a fake price.

from dev.

I'm a bit confused by the section between Lines 1036 to 1045. Why do we have this recoveryModeInMem and recoveryMode == false / true checks in the if statements at all?

Is the intention to save gas when the system just confirms the state of recoveryMode (though I think it does not cost storage gas to set something to true that is already true)?

How comes that our tests did not recognize this bug?
For example this test starting from line 723 in CDPManager_RecoveryModeTest.js:

it("liquidate(), with ICR > 110%, loan has lowest ICR, and StabilityPool is empty: does nothing", async () => {
   [...]
     const price = await priceFeed.getPrice()
    await cdpManager.checkTCRAndSetRecoveryMode(price)
    const recoveryMode = await cdpManager.recoveryMode()
    assert.isTrue(recoveryMode)

from dev.

Is the intention to save gas when the system just confirms the state of recoveryMode (though I think it does not cost storage gas to set something to true that is already true)?

I believe that's the intention, yes. Good question about the gas cost of setting something to the same value it already is.

How comes that our tests did not recognize this bug?

await cdpManager.recoveryMode() does return the correct value, however many CDPManager functions use the return value of checkTCRAndSetRecoveryMode(), which has the bug. Even then, it does return the correct value once, upon entering recovery mode. Subsequent calls return false however, until recovery mode is left and reentered again. So to catch this bug you'd have had to run 2 tests after triggering recovery mode.

from dev.

await cdpManager.recoveryMode() does return the correct value, however many CDPManager functions use the return value of checkTCRAndSetRecoveryMode(), which has the bug.

Oh, I see.

Subsequent calls return false however, until recovery mode is left and reentered again. So to catch this bug you'd have had to run 2 tests after triggering recovery mode.

Isn't that covered in the test through
await priceFeed.setPrice('100000000000000000000') which calls cdpManager.checkTCRAndSetRecoveryMode(price);?

from dev.

Isn't that covered in the test through
await priceFeed.setPrice('100000000000000000000') which calls cdpManager.checkTCRAndSetRecoveryMode(price);?

Huh. Good catch. It is indeed covered by the tests, and they're failing :D

I guess this shows why we need to sort out the automatic testing. We probably didn't notice the issue because the tests haven't been run in a long time.

from dev.

I made the contract tests run (excluding the error accumulation tests, because they use up too much time to be run all the time). You can check out the failures here:

https://github.com/cvalkan/cleverage/runs/599251082

from dev.

Regarding the cost of state updates:

I looked into it, and it's indeed as you said. Setting a bool (or any other type of storage) to the same value it already contains is very cheap.

If we truly wanted to save gas on toggling a bool-like variable, we should use e.g. an uint8 and toggle between 2 non-zero states (e.g. 1 and 2), since updating from zero to non-zero costs 20K while non-zero to anything is only 5K. Maybe an enum would work too and be more readable.

However, in this particular case (recoveryMode) we should probably remove the storage altogether.

from dev.

I made the contract tests run (excluding the error accumulation tests, because they use up too much time to be run all the time). You can check out the failures here:
Awesome!

Just a quick question. Why do certain tests have ticks, while others haven't?

Contract: CDPManager
    1) withdrawCLV(): reverts if withdrawal would pull TCR below CCR
    2) withdrawCLV(): reverts if system is in recovery mode
    ✓ openLoan(): reverts if withdrawal would pull TCR below CCR (204ms)
    3) openLoan(): reverts if system is in recovery mode
    ✓ withdrawColl(): reverts if system is in recovery mode (219ms)
    ✓ checkTCRandSetRecoveryMode(): changes recoveryMode to true if TCR falls below CCR (244ms)
    ✓ checkTCRandSetRecoveryMode(): leaves recoveryMode set to true if TCR stays less than CCR (264ms)

I looked into it, and it's indeed as you said. Setting a bool (or any other type of storage) to the same value it already contains is very cheap.

Thanks - good to know!

If we truly wanted to save gas on toggling a bool-like variable, we should use e.g. an uint8 and toggle between 2 non-zero states (e.g. 1 and 2), since updating from zero to non-zero costs 20K while non-zero to anything is only 5K. Maybe an enum would work too and be more readable.

Though, you get a gas refund 10K if you set it from non-zero to zero. But it's maybe better to flatten the costs by using an uin8 or enum type (if it really works).

from dev.

Just a quick question. Why do certain tests have ticks, while others haven't?

The numbered tests are the ones that are failing. The numbers help identify the failures :)

Though, you get a gas refund 10K if you set it from non-zero to zero. But it's maybe better to flatten the costs by using an uin8 or enum type (if it really works).

Oh, didn't know that! Neat!

from dev.

The numbered tests are the ones that are failing. The numbers help identify the failures :)

Got it!

from dev.

Though, you get a gas refund 10K if you set it from non-zero to zero.

Ah, so this is how gastoken.io works! Cool!

from dev.

Fixed in this commit: bb68f06

Originally I thought it made sense to have recovery mode as a state variable, and the shadowing variables were introduced to save gas on internal reads. The state variable is clearly unnecessary though - it led to this bug, and we can just compute recovery mode as needed.

The system now checks it with checkRecoveryMode, which returns a bool and does not write to storage.

All back-end tests are updated and passing now too.

from dev.