Open c4-bot-7 opened 6 months ago
0xSorryNotSorry marked the issue as sufficient quality report
0xSorryNotSorry marked the issue as duplicate of #937
Trumpero changed the severity to QA (Quality Assurance)
Trumpero marked the issue as grade-b
Trumpero marked the issue as grade-a
Hey, @Trumpero
This issue would need to be reconsidered.
eswak:
Somewhat similar to #1026 so I'm going to comment something along the same lines :
Acknowledging this, disagree with severity (imo it's informational).
This is the expected behavior, users are supposed to check each other and if the mintRatio go down, updateMintRatio of others so that they are not earning more rewards unduly. And if mintRatio is going up, users are expected to update their position to benefit from the new ratio. Ultimately the governance is a game of who has relatively more tokens, so the users act as keepers to each other to make sure no undue rewards are earned, and individually they are expected to do the actions needed to maximize their rewards.
The sponsor suggests that users should monitor if the mintRatio
changes and then execute updateMintRatio
for themselves and all other users in each term within the current market.
If the mintRatio
changes and there are 100 stakers, according to the suggestion, the subsequent 100 transactions would require all stakers to execute updateMintRatio
, which is deemed impractical and hard to achieve.
Also, as we've pointed out, who will prevent me or any user from claiming their reward right after the mintRatio update, before anyone else updates their Guild supply?
Claiming rewards at a better mint ratio can even happen unintentionally because of external factors, such as low gas provided or network congestion.
Lines of code
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L293-L315 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216-L290
Vulnerability details
Impact
When mintRatio is changed, a staker's guild amount should increase or decrease accordingly. However, this adjustment only occurs if the staker or someone else calls the
updateMintRatio()
function after the Governor set a new ratio. If a user unstakes or claims rewards after the mintRatio is updated butupdateMintRatio()
hasn't been called for them, they will receive rewards based on the old ratio, leading to a potential inconsistency.The protocol cannot depend on the assumption that all users will update their guild amounts after the governor sets a new mintRatio. This reliance could result in future rewards being based on inconsistent guild amounts.
Proof of Concept
updateMintRatio()
for the user.Coded PoC
Import
console
from"@forge-std/Test.sol”
Place the 3 tests in
SurplusGuildMinter.t.sol
Run them with:
Tools Used
Manual Review
Recommended Mitigation Steps
Make sure to update all users' guild amounts right after setting a new mintRatio. While we can't provide specific code advice due to issues in other parts of the contract, here's the main idea: When the governor changes the mintRatio, users should accumulate rewards based on their old guild amounts up to that moment. From the moment of the mintRatio update, the calculation of new rewards should be based on the new mint ratio. It's essential to distribute these rewards after the update and apply the logic in
updateMintRatio()
to ensure that new rewards reflect the updated guild amounts.Assessed type
Context