logos-co / staking

SNT Staking contracts
Creative Commons Zero v1.0 Universal
0 stars 4 forks source link

fix(StakeManager): initial MP should not increase when locking #94

Closed 0x-r4bbit closed 2 months ago

0x-r4bbit commented 2 months ago

When an account calls lock() on the StakeManager it ultimately called _mintInitialMP() which besides increasing the accounts currentMP, it also increases the accounts initialMP.

This seems incorrect, as initialMP should only increase at first stake.

Also, notice how inside lock() it always passes an amount of 0 to to _mintInitialMP(), so it only ever uses one code path of that function.

This commit adjust lock() such that it no longer uses _mintInitialMP(). Instead it makes use of _getMaxMPToMint based on the lock up time delta.

It also removes a test that claims that initialMP should increase on lock() and instead adds a test an a CVL rule ensure initialMP doesn't increase when locking.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

0x-r4bbit commented 2 months ago

Interesting, this change broke the MPcantBeGreaterThanMaxMP invariant. Investigating this now..

0x-r4bbit commented 2 months ago

The issue here is that, when we lock() we just mint the amount of MPs necessary for the given duration but we don't perform the max boost check. And since we're no longer increasing the initialMP when locking, we end up with currentMP > account.balance + account.initialMP.

0x-r4bbit commented 2 months ago

I'm closing this PR as invalid. As discussed with @3esmit offline, initialMP has to be updated when lock() is called to properly calculate the max cap MP later on.

It turned out that initialMP is just a misleading name. Hence, we're proposing the changes done in #95 which renames it to bonusMP.