User minting `FPS` can get grieved by equity loss event

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L244-L247

Vulnerability details

minting in onTokenTransfer handles the case equity <= amount as the first deposit, minting it 1000 shares no matter the amount of ZCHF transferred

File: Equity.sol
244:         require(equity >= MINIMUM_EQUITY, "insuf equity"); // ensures that the initial deposit is at least 1000 ZCHF
245: 
246:         // Assign 1000 FPS for the initial deposit, calculate the amount otherwise
247:         uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);

The issue is that there is another case scenario in which equity <= amount: if a loss happens and equity stands at a negative value (see the example given here).

In such case, if amount is large enough to bring back equity above water here, it will result in equity <= amount being true, minting the user only 1000 shares.

Impact

An innocent user calling Equity.onTokenTransfer() just after a huge equity loss - which triggered Frankencoin.notifyLoss, will get fewer shares than expected.

Note that as the transactions can happen in the same block, there is nothing protecting users from this happening.

The big difference with this scenario is that the victim did not intend to do this - also, they will not be able to bootstrap the system after this because of the check line 311.

Proof Of Concept

Run the test test_Audit_Equity_UnfairLoss in the following gist: https://gist.github.com/joestakey/fbc954de53b7e018f5121212488e847c

run it once as it is, this should result in:

FPS charlieBalance:  1429120543707280923000 // 1429 shares

Then toggle the block marked with TOGGLE on - which simulates a loss.

This should result in:

FPS charlieBalance:  1000000000000000000000 // 1000 shares

The equity loss resulted in Charlie receiving less shares than expected

Tools Used

Manual Analysis

Mitigation

onTokenTransfer should introduce a minSharesOut parameter to protect users from this scenario.

Users will be able to estimate the number of shares they would receive using calculateShares(), and use this as a basis for how many shares they allow to receive.

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[luziusmeisser]

I see this as an edge case of #976 . It could be solved by adding slippage protection.

[luziusmeisser]

Will add slippage protection to future versions.

[c4-sponsor]

luziusmeisser marked the issue as sponsor confirmed

[c4-judge]

hansfriese marked the issue as duplicate of #396

[c4-judge]

hansfriese marked the issue as duplicate of #396

[c4-judge]

hansfriese marked the issue as satisfactory