Frankencoin - BenRai's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L109-L114 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L252-L276 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L280-L288

Vulnerability details

Impact

Once a position has surpassed its start time it can no longer be vetoed by calling the function deny(). The only way to remove the position is to challenge it. When a position is challenged, and the challenge is successful the challenger is paid the challenger-reward based on the potential damage the position could have done. If the bid of the bidder for the challenge is not enough to cover the repayment and the challenger-reward the difference is calculated and covered by the reserve using notifyLoss(). If the reserve does not have enough funds to cover the reward the missing amount is just minted and sent to the attackers wallet. If the reward becomes to high funds of the reserve can be lost and all holders of the Frankencoin Pool Shares will lost their money. In addition a depegg might happen if in addition there were additional ZCHF minted to cover the reward.

Proof of Concept

A minter could open a position for eg. 1 ETH and price it reasonably (eg. 1000 ZCHF) so the position does not get vetoed by anyone. Once the start time has passed, he changes the price to 1 billion ZCHF and challenges his own position (from another wallet). Since the challenge will be successful because no one will pay 1 billion ZCHF for 1 ETH he will get a reward of 2% of the potential damage of 1 Billion ZCHF. With a CHALLENGER_REWARD of 2% this would be 20 million ZCHF. Since this amount cannot be covered by the price paid for the 1 ETH the reserves will cover the difference and thereby drain funds from the reserve. If the reward is high enough all the funds of the reserve will be drained or even worth, the funds that the reserve cannot cover will just be minted and send to the attackers wallet.

When the attacker sells/swaps all this additional ZCHF that where minted without the backing of collateral the ZCHF will depegg and all holders of ZCHF will loose their money since the ZCHF becomes worthless.

Recommended Mitigation Steps

The reward could be caped at a certain value, eg. 1000 ZCHF so challenging positions is still profitable for the challenger but the amount that is rewarded does not get out of hand. Also, the code could be changed so a position where the price has been increased can be denied again. This way the system can be protected against such an attack.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L112 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L263 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L373-L376

Vulnerability details

Impact

If a minter has a positive outlook on ZCHF and suggest a position with a long duration and the position is denied, the cool down for the position is set to the expiration date. Since the owner of a position with a cooldown cannot withdraw collateral the collateral he already deposited when creating the position will be looked untill the position expires.

Proof of Concept

A Minter creates a position with a duration of 1 year and deposits the collateral into the position. A bad actor (eg. a competing CHF stable coin) that holds 3% of the reserve shares denies the position and thereby sets the cooldown to the expiration date 1 year from now. The collateral of the position owner is now locked for 1 year since one cannot withdraw collateral from a position with a cooldown. Trust in the ZCHF sinks since there is no way to make sure that the new positions can not be denied by the bad actor. This could mean, that no new positions are suggested and the marketshare of ZCHF can no longer grow.

Recommended Mitigation Steps

Instead of setting the cool down to the expiration date there should be an additional variable that tracks if a position was denied or not. If a position was denied the owner should be able to withdraw the collateral right away.

Follow convention for the order of events, errors, modifiers and Interfaces

Errors, events, modifiers and interfaces in the following contracts are scattered all over the code and/or sit below the functions. To meet the convention and have a better overview they should be grouped together and put above the functions of the contract. The affected contracts are:

Frankencoin.sol MintingHub..sol Position.sol

There some typos or missing words in comments

file: contracts/ MintingHub.sol

258: // notify the position that will send the collateral to the bidder Should be: notify the position that the collateral will be send to the bidder

file: contracts:/ Position.sol

358: * This is also a good creterion when deciding whether it should be shown in a frontend. Criterion should be Critirion

file: contracts/MintingHub.sol

153: * have the liquidity available to bid a sufficient amount. With this function, the can split of smaller slices of Should be: With this function, the position owner can split / or “anyone can split”

file: Frankencoin.sol

56: * Initiates the Frankencoin with the provided minimum application period for new plugins Comment should not say plugins but minters for better understanding 188: * Design rule: Minters calling this method are only allowed to so for tokens amounts they previously minted with the same _reservePPM amount.

Should be: … only allowed to do so for…

Event Name should be PostponedReturn() with only one capital P

file: contracts/MintingHub.sol

52: event PostPonedReturn(address collateral, address indexed beneficiary, uint256 amount);

Comment for example should not be in the code but above the code

To be consistent with the other comments the comments for the example should be above the code not within the code of the function.

fiel: contracts/Frankencoin.sol

lines 235 - 240

Remove TODOs from comments

file: contracts/MintingHub.sol

74: * TODO: in future versions, it might be better to fix the interest and not the fee

transfareAndCall() should not be “override” since it is not inherited from ERC20

transfareAndCall() is no inhareded from IERC20 so it should no have the keyword “override” in it

file: contract/ERC20.sol 162: function transferAndCall(address recipient, uint256 amount, bytes calldata data) external override returns (bool) {

Comments are not consistent with the code

In the comment in line 36-38 two function are mentioned that do not exist in the contract. For clarity this lines should be removed since there are no decreaseAllowance and increaseAllowance functions in the contract.

file: contracts/ERC20.sol

36-38: * Finally, the non-standard decreaseAllowance and increaseAllowance

• functions have been added to mitigate the well-known issues around setting
• allowances. See IERC20.approve.

Function restructurCapital() can run out of gas

If the address array for the address to wipe gets to long the function might run out of gas and therefore restructuring the capital might not be possible. File: equity.sol 312: for (uint256 i = 0; i<addressesToWipe.length; i++){

Move check that might revert using a function from another contract to the top of the function

The function launchChallenge() in the file MintingHub.sol checks in line 145 with the function notifyChallengeStarted() if the amount the user is challenging is equal or bigger the minimumCollateral of the position. By moving this check from line 145 below line 141 will save the user gas in case he accidentally challenged a position that is smaller than the minimumCollateral. File: contracts/MintingHub.sol 145: position.notifyChallengeStarted(_collateralAmount);

Use calldata instead of storage to save gas

Using calldata for arrays needs less gas than using memory.

file: contracts/MintingHub.sol

188: function minBid(Challenge storage challenge) internal view returns (uint256) { 287: function returnCollateral(Challenge storage challenge, bool postpone) internal {

Remove function end() in line 235

The function end() in line 235 is for saving gas by making an internal call to the function end() in line 252. But since the function end() in line 252 is not internal but public calling the function end() in line 235 does not save gas but cost more than calling end() in line 252. Therefore to save gas on deployment the function end() in line 235 can be removed.

file: contracts/MintingHub.sol

235: function end(uint256 _challengeNumber) external { end(_challengeNumber, false); }

Use a variable for minBid(challenge) instead of calling it twice

The value for minBid(challenge) can be saved in a variable to save gas instead of calling it twice in one function

file: contracts/MintingHub.sol

216: if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge))

Saving gas for the user by failing early

Some checks(if/require) can be moved up in the functions to save gas for the user in case the checks fail.

file: contracts/MintingHub.sol

109: require(_initialCollateral >= _minCollateral, "must start with min col")

• can be moved above line 107 to save gas in case check fails 171: require(challenge.size >= min); 172: require(copy.size >= min);
• can both be moved above line 167 to save gas for the user in case check fails 216: if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));
• can be moved to the top of the function to save gas for the user in case it reverts

file: contracts/Position.sol

81: if (price > _price) revert InsufficientCollateral();

• setOwner(owner) from line 78 can be moves below this statement to save gas for the user in case the if check fails