Frankencoin - yixxas's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L241-L255 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L266-L270

Vulnerability details

Impact

Initial minter can steal from the next investor. The amount stolen is based on how much the next investor chooses to invest.

Proof of Concept

In observing the onTokenTransfer() function, we notice that if equity <= amount, then only 1000 shares will be minted to from address no matter the amount deposited. This is used to mint 1000 FPS for the initial deposit, otherwise calculateSharesInternal() is used to calculate shares.

function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {
	require(msg.sender == address(zchf), "caller must be zchf");
	uint256 equity = zchf.equity();
	require(equity >= MINIMUM_EQUITY, "insuf equity"); // ensures that the initial deposit is at least 1000 ZCHF

	// Assign 1000 FPS for the initial deposit, calculate the amount otherwise
	uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);
	_mint(from, shares);
	emit Trade(msg.sender, int(shares), amount, price());

	// limit the total supply to a reasonable amount to guard against overflows with price and vote calculations
	// the 128 bits are 68 bits for magnitude and 60 bits for precision, as calculated in an above comment
	require(totalSupply() < 2**128, "total supply exceeded");
	return true;
}

calculateSharesInternal sets newTotalShares to 1000e18 if totalShares < 1000. This is irrespective of amount of investment.

function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {
	uint256 totalShares = totalSupply();
	uint256 newTotalShares = totalShares < 1000 * ONE_DEC18 ? 1000 * ONE_DEC18 : _mulD18(totalShares, _cubicRoot(_divD18(capitalBefore + investment, capitalBefore)));
	return newTotalShares - totalShares;
}

A malicious first depositor can steal from the next investor due to this.

Steps of attack:

1. Malicious attacker mints 1000 shares
2. Malicious attacker waits for the next investor to invest and deposit ZCHF. He frontruns the transaction and then calls redeem() to redeem 1 share. One condition here is that sufficient blocks must have passed such that redemption is possible.
3. Investor is then minted 1 share and attacker holds 999 shares. If the amount invested in this transaction is large, attacker would benefit significantly.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider disallowing redemption of shares such that totalSupply() < 1000.

QA - Low-01

3% Quorum seems to be too low. Being a qualified user has significant privileges. A qualified user can perpetually veto the addition of a new minter and can also restructure the system during dire situations. Considering that chain delegation is used, it is not difficult to reach a 3% quorum. In fact, there can be many delegates who will reach this threshold.

For example, if we only have 500 votes evenly distributed across 5 users A,B,C,D,E. Total votes is 9000.

A delegates to B who delegates to C who delegates to D who delegates to E.

In this case, C,D,E all have enough votes from their delegatees. C has 300, D has 400, E has 500. Because votes are stacked cumulatively and the delegate inherits all from delegatee without actually losing any votes of itself, quorum should be set to a higher amount.

8-10% seems a better choice of QUORUM.