Open c4-bot-4 opened 6 months ago
0xSorryNotSorry marked the issue as sufficient quality report
0xSorryNotSorry marked the issue as duplicate of #937
The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.
0xSorryNotSorry marked the issue as high quality report
Trumpero changed the severity to QA (Quality Assurance)
Trumpero marked the issue as grade-b
Trumpero marked the issue as grade-a
Lines of code
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L319 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L293 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216
Vulnerability details
Impact
The user can stake credit tokens using the SurplusGuildMinter contract. The guildAmount to mint is calculated using the mintRatio and the credit tokens amount:
Then based on the calculated guildAmount, the corresponding rewards can be obtained for the user code line 250:
The issue arises when the
mintRatio
is updated, causing the user's rewards to not be calculated correctly because theguildAmount
is not updated based on the newmintRatio
. The SurplusGuildMinter::updateMintRatio() function helps to update the new user's guildAmount based on the newmintRatio
however this function is not required to be called by the user and there are not comments/doc which mention thatupdateMintRatio()
will be called to all users after amintRatio
change.In the other hand there is a comment that mentions the following:
That is, when
rewardRatio
is changed with the function SurplusGuildMinter::setRewardRatio(), it is necessary to call the rewards of all users via the SurplusGuildMinter::getRewards() function, however nothing is mentioned whenmintRatio
is updated.Proof of Concept
Please consider the next scenario where the
mintRatio
increases from2e18
to3e18
:userA
stakes10e18 creditTokens
and20e18 guild tokens
are mintedguildAmount = (_mintRatio * amount) / 1e18; = (2e18 * 10e18) / 1e18 = 20e18
mintRatio
is updated via setMintRatio() tomintRatio=3e18
.userA
stakes another10e18 creditTokens
and30e18 guild tokens
are minted, based on the formulaguildAmount = (_mintRatio * amount) / 1e18; = (3e18 * 10e18) / 1e18 = 30e18
. Now he has20e18 + 30e18 = 50e18 guildTokens
.That is incorrect because
userA
should have60e18 guildTokens
insted of50e18 guildTokens
. Since the newmintRatio is 3e18
and the user has staked20e18 creditTokens
, the guild amount should beguildAmount = (mintRatio * amount) / 1e18; = (3e18 * 20e18) / 1e18 = 60e18
. I created the following test where the correct guild amount is updated once the SurplusGuildMinter::updateMintRatio() is called the problem is that theupdateMintRatio()
function is not forced to be called and the user can get more rewards than it should be.Consider the next scenario where the
mintRatio
decreases from3e18
to2e18
:userA
stakes10e18 creditTokens
and30e18 guild tokens
are minted.guildAmount = (_mintRatio * amount) / 1e18; = (3e18 * 10e18) / 1e18 = 30e18
mintRatio
is updated tomintRatio=2e18
.userA
claims rewards via the SurplusGuildMinter::getRewards() function using a not updatedguildAmount=30e18
In the above scenario,
userA
will get more rewards because rewards will be calculated usingguildAmount=30e18
, that's is incorrect because the newmintRatio is 2e18
soguildAmount = (_mintRatio * amount) / 1e18; = (2e18 * 10e18) / 1e18 = 20e18
Tools used
Manual review
Recommended Mitigation Steps
The SurplusGuildMinter::updateMintRatio() function is not enforced to be called once the
mintRatio
is changed, so the recommendation is to update theguildAmount
if the mintRatio has changed at the end of the SurplusGuildMinter::getRewards() function, so now the user is forced to update theguildAmount
andmintRatio
and get fair rewards:Assessed type
Context