Frankencoin - DishWasher's results

Lines of code

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

Vulnerability details

Impact

In certain cases, buying X FPS at once is not the same as first buying X-Y and then Y FPS. This implies, that buyers will not always receive the correct amount of pool shares, leading them to a loss of money and decreased trust in the system.

Proof of Concept

Consider the following scenario: Frankencoin is in debt by 500 ZCHF, due to outstanding position reserves of 500 and an empty overall reserve. A user wants to buy FPS and also help the system to recover. He pays 10000 ZCHF. As the capital before investment is below 1000, he will receive 1000 pool shares. In a different scenario, the same user first transfers 1500 ZCHF, bringing the system back to a state of minimum equity and receiving 1000 FPS and then in a second transaction transfers 8500 ZCHF. He will receive additional pool shares for this second transaction, as the number of current pool shares is not less than 1000, thereby increasing the total number of pool shares received to a much higher value than in the first scenario.

Tools Used

Recommended Mitigation Steps

Implement onTokenTransfer, such that it makes a case distinction for received money. First filling up the pool reserves until 1000 FPS are reached, crediting the corresponding 1000 FPS to the caller. Then, after that proceed with accounting for the remaining received ZCHF as done when there are more than 1000 pool shares.

QA report

1.) no contract is implementing IERC677. - Equity.sol and StablecoinBridge both implement onTokenTransfer but dont inherit the method from the ERC677 interface - fix: inherit from IERC677 and add override keyword to methods

2.) allowanceInternal function in ERC20.sol seems unnecessary given allowance function.

3.) the way in which the current implementation is keeping track of votes looses the distinction between different owned shares (see adjustRecipientVotesAnchor) - if I own some shares for 1000 blocks and others for 100 blocks I would like to sell or move those that I own for a shorter amount of time. At the moment, whenever new shares are bought, the holding time is averaged, making this desired behavior not possible.

4.) In general, it might be a good idea to create a Minter interface that every proposed minter has to implement. This would create more clarity in the codebase and also more control over the specific implementation of minters.

5.) in MathUtils.sol cubicRoot function power3 function is not used although it is implemented

6.) add description of _initPeriodSeconds parameter in openPosition function MintingHub

7.) minBid() in the MintingHub.sol contract is supposed to be the minimal size for a next bid. However, if that next bid is large enough to avert the challenge it is accepted even though it might be smaller than minBid()

Equity votes(sender, helpers) function inefficient uniqueness check (https://github.com/code-423n4/2023-04-Frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L196-L198) Instead, create alreadyHelped mapping (address -> bool) setting it to true whenever a helpers vote are added to the total votes and reverting if it is true already.

suggestMinter function, do not check if totalSupply() > 0 twice, but only once before performing the FeeTooLow and periodTooShort checks (https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L84-L85)