code423n4 commented 1 year ago

Lines of code

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

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 position is opened on mintingHub, which increases equity by transferring zchf to the reserve:

File: MintingHub.sol
108: zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);

Impact

If a position is opened before Equity shares have started to be minted, the following scenario can happen:

Frankencoin is deployed
a new minter is registered via suggestMinter()
the owner calls openPosition to open a position - note that this can be done instantly, there is no waiting time to open a position.
The reserve now has zchf.
Bob calls zchf.transferAndCall(address(zchf.reserve()), 0, "")
Because equity.totalSupply == 0, newTotalShares is set as 1000e18 here
Bob receives 1000 FPS without having transferred any zchf. He will be able to redeem it against zchf in the future, stealing from other FPS holders

Proof Of Concept

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

This shows the steps enounced above, and should result in:

Logs:
  FPS aliceBalance:    442249570307403220000
  FPS bobBalance:      1000000000000000000000
  zchf redeemed by bob 2913502758233489298000

Tools Used

Manual Analysis

Mitigation

onTokenTransfer should be refactored to revert if amount <= 1000 when totalSupply == 0

File: Equity.sol
244: require(equity >= MINIMUM_EQUITY, "insuf equity"); // ensures that the initial deposit is at least 1000 ZCHF
+    if(totalSupply() == 0) require(amount >= 1000e18);
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);

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #746

c4-judge commented 1 year ago

hansfriese changed the severity to QA (Quality Assurance)