search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

xiaoming90 - Large amounts of points can be minted virtually without any cost #187

Open sherlock-admin opened 7 months ago

sherlock-admin commented 7 months ago

xiaoming90

high

Large amounts of points can be minted virtually without any cost

Summary

Large amounts of points can be minted virtually without any cost. The points are intended to be used to exchange something of value. A malicious user could abuse this to obtain a large number of points, which could obtain excessive value and create unfairness among other protocol users.

Vulnerability Detail

When depositing stable collateral, the LPs only need to pay for the keeper fee. The keeper fee will be sent to the caller who executed the deposit order.

When withdrawing stable collateral, the LPs need to pay for the keeper fee and withdraw fee. However, there is an instance where one does not need to pay for the withdrawal fee. Per the condition at Line 120 below, if the totalSupply is zero, this means that it is the final/last withdrawal. In this case, the withdraw fee will not be applicable and remain at zero.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/StableModule.sol#L96

File: StableModule.sol
096:     function executeWithdraw(
097:         address _account,
098:         uint64 _executableAtTime,
099:         FlatcoinStructs.AnnouncedStableWithdraw calldata _announcedWithdraw
100:     ) external whenNotPaused onlyAuthorizedModule returns (uint256 _amountOut, uint256 _withdrawFee) {
101:         uint256 withdrawAmount = _announcedWithdraw.withdrawAmount;
..SNIP..
112:         _burn(_account, withdrawAmount);
..SNIP..
118:         // Check that there is no significant impact on stable token price.
119:         // This should never happen and means that too much value or not enough value was withdrawn.
120:         if (totalSupply() > 0) {
121:             if (
122:                 stableCollateralPerShareAfter < stableCollateralPerShareBefore - 1e6 ||
123:                 stableCollateralPerShareAfter > stableCollateralPerShareBefore + 1e6
124:             ) revert FlatcoinErrors.PriceImpactDuringWithdraw();
125: 
126:             // Apply the withdraw fee if it's not the final withdrawal.
127:             _withdrawFee = (stableWithdrawFee * _amountOut) / 1e18;
128: 
129:             // additionalSkew = 0 because withdrawal was already processed above.
130:             vault.checkSkewMax({additionalSkew: 0});
131:         } else {
132:             // Need to check there are no longs open before allowing full system withdrawal.
133:             uint256 sizeOpenedTotal = vault.getVaultSummary().globalPositions.sizeOpenedTotal;
134: 
135:             if (sizeOpenedTotal != 0) revert FlatcoinErrors.MaxSkewReached(sizeOpenedTotal);
136:             if (stableCollateralPerShareAfter != 1e18) revert FlatcoinErrors.PriceImpactDuringFullWithdraw();
137:         }

When LPs deposit rETH and mint UNIT, the protocol will mint points to the depositor's account as per Line 84 below.

Assume that the vault has been newly deployed on-chain. Bob is the first LP to deposit rETH into the vault. Assume for a period of time (e.g., around 30 minutes), there are no other users depositing into the vault except for Bob.

Bob could perform the following actions to mint points for free:

Each attack requires 20 seconds (10 + 10) to be executed. Bob could rinse and repeat the attack until he was no longer the only LP in the system, where he had to pay for the withdraw fee, which might make this attack unprofitable.

If Bob is the only LP in the system for 30 minutes, he could gain 9000e18 points ((30 minutes / 20 seconds) * 100e18 ) for free as Bob could get back his keeper fee and does not incur any withdraw fee. The only thing that Bob needs to pay for is the gas fee, which is extremely cheap on L2 like Base.

File: StableModule.sol
61:     function executeDeposit(
62:         address _account,
63:         uint64 _executableAtTime,
64:         FlatcoinStructs.AnnouncedStableDeposit calldata _announcedDeposit
65:     ) external whenNotPaused onlyAuthorizedModule returns (uint256 _liquidityMinted) {
66:         uint256 depositAmount = _announcedDeposit.depositAmount;
..SNIP..
70:         _liquidityMinted = (depositAmount * (10 ** decimals())) / stableCollateralPerShare(maxAge);
..SNIP..
75:         _mint(_account, _liquidityMinted);
76: 
77:         vault.updateStableCollateralTotal(int256(depositAmount));
..SNIP..
82:         // Mint points
83:         IPointsModule pointsModule = IPointsModule(vault.moduleAddress(FlatcoinModuleKeys._POINTS_MODULE_KEY));
84:         pointsModule.mintDeposit(_account, _announcedDeposit.depositAmount);

Impact

Large amounts of points can be minted virtually without any cost. The points are intended to be used to exchange something of value. A malicious user could abuse this to obtain a large number of points, which could obtain excessive value from the protocol and create unfairness among other protocol users.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/StableModule.sol#L96

Tool used

Manual Review

Recommendation

One approach that could mitigate this risk is also to impose withdraw fee for the final/last withdrawal so that no one could abuse this exception to perform any attack that was once not profitable due to the need to pay withdraw fee.

In addition, consider deducting the points once a position is closed or reduced in size so that no one can attempt to open and adjust/close a position repeatedly to obtain more points.

sherlock-admin commented 6 months ago

2 comment(s) were left on this issue during the judging contest.

0xLogos commented:

low/info, points != funds

takarez commented:

invalid

nevillehuang commented 6 months ago

@rashtrakoff Any reason why this issue was disputed? What are the points FMP for?

I believe large amount of points shouldn't be freely minted.

rashtrakoff commented 6 months ago

@nevillehuang , this is a good find imo but since we are going to be the first depositors as well as creators of leverage positions as part of protocol initialisation I wouldn't believe this is something we are concerned about. Furthermore, the points have no monetary value (at least not something we are going to assign) and there are costs associated with doing looping (keeper fees, possible losses due to price volatility etc.). Cc @itsermin @D-Ig .

nevillehuang commented 6 months ago

@rashtrakoff I will be maintaining as medium severity, even though points currently do not hold value, I believe it is not intended to allow free minting of points freely given it will hold some form of incentives in the future, so I believe it breaks core contract functionality. From my understanding, being the first depositor is only given as an example and is not required as shown in other issues such as #44 and #141.

0xLogos commented 6 months ago

Escalate

Should be info

the points have no monetary value there are costs associated with doing looping

sherlock-admin2 commented 6 months ago

Escalate

Should be info

the points have no monetary value there are costs associated with doing looping

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

santipu03 commented 6 months ago

Agree with @0xLogos.

Moreover, the withdrawal fee will make the attacker lose value with each withdrawal, making the attack unfeasable.

Even in the improbable case that the attacker is a sophisticated bot that can act as a keeper and the withdrawal fee is not activated, the probability would be low with the impact being low/medium. Therefore, the overall severity should be low.

securitygrid commented 6 months ago

This issue is valid. According to rules:

Loss of airdrops or liquidity fees or any other rewards that are not part of the original protocol design is not considered a valid high/medium

FMP is part of the original protocol design. It's an incentive for users.

0xcrunch commented 6 months ago

Sponsor is OK with issues related to FMP if they are not relevant to the overall functioning of the protocol.

https://discord.com/channels/812037309376495636/1199005620536356874/1200372130253115413

xiaoming9090 commented 6 months ago

If points are not an incentive or something of value to the user, then there is no purpose for having a points system in the first place. The obvious answer is that the points will not be worthless because it makes no sense for users to hold something that is worthless. With that, points should be considered something of value, and any bugs, such as infinity minting of points/values, should not be QA/Low.

0xcrunch commented 6 months ago

This kind of issue has no impact to the overall functioning of the protocol, so it is acceptable as stated by sponsor in the public channel.

nevillehuang commented 6 months ago

Agree with @xiaoming9090, I believe there is no logical reason why this should be allowed in the first place.

@rashtrakoff What is the intended use case of points? It must have some incentive attached to it (even if its in the future), so users should never be getting points arbitrarily.

itsermin commented 6 months ago

Thanks for your inputs here @xiaoming9090 @nevillehuang

I discussed this with @rashtrakoff today. Even though there's a cost associated with looped mints (trading fees). Being able to mint a large number of FMP is not ideal. The user could alao potentially LP on the UNIT side to minimise their downside.

We're looking at a couple of options: a) put a daily cap on the number of available FMP b) remove the trade volume minting altogether

0xcrunch commented 6 months ago

Rewarding an issue publicly known as acceptable is unfair to watsons who didn't submit the issue out of respecting of Sherlock rules.

Czar102 commented 6 months ago

I'd normally consider this a valid issue given that the points may have some value, but given the sponsor's message referenced in https://github.com/sherlock-audit/2023-12-flatmoney-judging/issues/187#issuecomment-1956258407, I think this should be considered informational.

Planning to accept the escalation and invalidate the issue.

nevillehuang commented 6 months ago

@Czar102 Fair enough, given the new hierachy of truth in place, I believe this can be low severity, unless watsons have any contest details and/or protocol documentation indicating a incentivized use case of points. @xiaoming9090 @securitygrid

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

novaman33 commented 6 months ago

@Czar102 , I believe there are several code comments that suggest that points are incentive: In PointsModule.sol 

/// @title PointsModule
/// @author dHEDGE
/// @notice Module for awarding points as an incentive.

And before owner mintTo function:

/// @notice Owner can mint points to any account. 
This can be used to distribute points to competition winners and other reward incentives.
    ///         The points start a 12 month unlock tax (update unlockTime).

These state that points will be used as an encouragement, meaning they will either be something of value or be used to obtain something of value. I cannot agree that being able to obtain large amounts of points is a low severity case, given the comments in the code, which in the sherlock's Hierarchy of truth have more weight than protocol answers on the contest public Discord channel.

nevillehuang commented 6 months ago

@Czar102 , I believe there are several code comments that suggest that points are incentive: In PointsModule.sol

/// @title PointsModule
/// @author dHEDGE
/// @notice Module for awarding points as an incentive.

And before owner mintTo function:

/// @notice Owner can mint points to any account. 
This can be used to distribute points to competition winners and other reward incentives.
    ///         The points start a 12 month unlock tax (update unlockTime).

These state that points will be used as an encouragement, meaning they will either be something of value or be used to obtain something of value. I cannot agree that being able to obtain large amounts of points is a low severity case, given the comments in the code, which in the sherlock's Hierarchy of truth have more weight than protocol answers on the contest public Discord channel.

Good point, in that case, I believe this issue should remain medium severity, given code comments (as seen here and here has a greater significance than discord messages as shown in the hierarchy of truth above

Czar102 commented 6 months ago

I believe considering something an incentive doesn't imply this functionality being important enough for issues regarding it to be considered valid, while the sponsor's comment directly specified whether the issues of the type are valid. Also, these comments don't contradict the sponsor's comment at all.

I stand by the previous proposition to invalidate the issue.

novaman33 commented 6 months ago

@Czar102 the contracts in scope are stated in the readMe. The sponsor said " we can be ok with issues with the same.", by which they state issues related to the points module are out of scope. I cannot understand why the points module is in scope in the first place. Given the Hierarchy of truth I believe points module are still in scope.

nevillehuang commented 6 months ago

@Czar102 I don't quite get your statement, it was already stated explicitly in code comments of the contract that points are meant to have an incentivized use case. So by hierarchy of truth, this is clearly a medium severity issue (and maybe can even be argued as high severity). Whatever it is, I will respect your decision, but hoping for a better clarification.

Czar102 commented 6 months ago

Given the hierarchy of truth, I think this issue is indeed a valid Medium.

Planning to reject the escalation and leave the issue as is.

Evert0x commented 6 months ago

Result: Medium Has Duplicates

sherlock-admin2 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin4 commented 6 months ago

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/294.