Frankencoin - joestakey's results

Lines of code

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

Vulnerability details

Equity.restructureCapTable allows qualified FPS holders to restructure the system, that is: burning shares of other holders that did not participate in putting equity above water.

File: Equity.sol
309: function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {
310:         require(zchf.equity() < MINIMUM_EQUITY);
311:         checkQualified(msg.sender, helpers);
312:         for (uint256 i = 0; i<addressesToWipe.length; i++){
313:             address current = addressesToWipe[0];//@audit should be addressesToWipe[i]
314:             _burn(current, balanceOf(current));
315:         }
316:     }

The issue is that the loop sets on each iteration the address to remove as addressesToWipe[0], instead of addressesToWipe[i].

Impact

restructureCapTable() does not perform its function: removing atomically all the "passive" shareholders. This not only means increased gas cost - as the qualified holder will need to call Equity.restructureCapTable() N times to remove N holders, but also makes the whole procees cumbersome if the function needs to be called a high number of times

Proof Of Concept

Run the test test_Audit_Equity_RestructureIncorrect from the following gist: https://gist.github.com/joestakey/fbc954de53b7e018f5121212488e847c

Only the first address gets wiped out.

Tools Used

Manual Analysis, Foundry

Mitigation

File: Equity.sol
309: function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {
310:         require(zchf.equity() < MINIMUM_EQUITY);
311:         checkQualified(msg.sender, helpers);
312:         for (uint256 i = 0; i<addressesToWipe.length; i++){
+313:             address current = addressesToWipe[i];
-313:             address current = addressesToWipe[0];
314:             _burn(current, balanceOf(current));
315:         }
316:     }

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L352 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L269

Vulnerability details

A successful challenge can be ended via MintingHub.end(). This transfers challenge.size collateral back to the challenger, before repaying the challenge and paying the challenger the reward.

In this call, position.notifyChallengeSucceeded is called. This transfers the position's collateral to the highest bidder.

File: Position.sol
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

File: Position.sol
268: function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
269:         IERC20(collateral).transfer(target, amount);
270:         uint256 balance = collateralBalance();
271:         if (balance < minimumCollateral){
272:             cooldown = expiration;
273:         }
274:         emitUpdate();
275:         return balance;
276:     }

The issue is that in case of a token with blacklist (USDC, USDT), if the bidder is blacklisted before the end() call, the transfer line 269 will revert

Impact

end() will always revert for this challenge. The challenge cannot be ended and the challenger will never retrieve their size collateral.

Tools Used

Manual Analysis

Mitigation

end() already handles the case where the challenger is blacklisted by such tokens. Add a similar functionality in notifyChallengeSucceeded() for the internalWithdrawCollateral() internal call.

Lines of code

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

Vulnerability details

FPS holders can redeem their shares against zchf using redeem()

File: Equity.sol
276: function redeem(address target, uint256 shares) public returns (uint256) {//@audit no slippage, calculateProceeds() can return 0
277:         require(canRedeem(msg.sender));
278:         uint256 proceeds = calculateProceeds(shares);
279:         _burn(msg.sender, shares);
280:         zchf.transfer(target, proceeds);
281:         emit Trade(msg.sender, -int(shares), proceeds, price());
282:         return proceeds;
283:     }

The issue is that there is no slippage in this function. As the amount of zchf computed in calculateProceeds() is a function of equity, events happening in the same block before the redeem() call can affect the amount of zchf received by the user

Impact

Users can receive less than expected.

But if a big equity loss happens resulting in zchf.equity returning 0, calculateProceeds will return 0, meaning the user will see their shares being burnt without receiving any zchf.

While it can be argued that the user shares were worth nothing due to the equity loss, in the case of the "brave soul" ready to save the protocol - mentioned in the case scenario here, they will have lost the ability and incentive to do so.

Tools Used

Manual Analysis

Mitigation

Add a slippage parameter in redeem()

Users will be able to estimate how much they can expect using calculateProceeds().

Lines of code

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

Vulnerability details

minting in onTokenTransfer handles the case equity <= amount as the first deposit, minting it 1000 shares no matter the amount of ZCHF transferred

File: Equity.sol
244:         require(equity >= MINIMUM_EQUITY, "insuf equity"); // ensures that the initial deposit is at least 1000 ZCHF
245: 
246:         // Assign 1000 FPS for the initial deposit, calculate the amount otherwise
247:         uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);

The issue is that there is another case scenario in which equity <= amount: if a loss happens and equity stands at a negative value (see the example given here).

In such case, if amount is large enough to bring back equity above water here, it will result in equity <= amount being true, minting the user only 1000 shares.

Impact

An innocent user calling Equity.onTokenTransfer() just after a huge equity loss - which triggered Frankencoin.notifyLoss, will get fewer shares than expected.

Note that as the transactions can happen in the same block, there is nothing protecting users from this happening.

The big difference with this scenario is that the victim did not intend to do this - also, they will not be able to bootstrap the system after this because of the check line 311.

Proof Of Concept

Run the test test_Audit_Equity_UnfairLoss in the following gist: https://gist.github.com/joestakey/fbc954de53b7e018f5121212488e847c

run it once as it is, this should result in:

FPS charlieBalance:  1429120543707280923000 // 1429 shares

Then toggle the block marked with TOGGLE on - which simulates a loss.

This should result in:

FPS charlieBalance:  1000000000000000000000 // 1000 shares

The equity loss resulted in Charlie receiving less shares than expected

Tools Used

Manual Analysis

Mitigation

onTokenTransfer should introduce a minSharesOut parameter to protect users from this scenario.

Users will be able to estimate the number of shares they would receive using calculateShares(), and use this as a basis for how many shares they allow to receive.

Report

Low Issues

L-1 Misleading liquidation price documentation

As per the documentation:

Anyone can challenge a position if they believe that the liquidation price is below the true value of the collateral

But looking at the invariant in Position it is the opposite: a position should be challenged if its price is above the true value of collateral.

e.g:

if collateral is USDT and price == 2e18, the position should be challenged (say the position is collateralized with 1M USDT and minted 2M ZCHF, it is undercollateralized if we look at the current CHF/USD pair)

File: Position.sol
282:     function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {
283:         if (collateralReserve * atPrice < minted * ONE_DEC18) revert InsufficientCollateral();
284:     }

L-2 Unsafe cast may overflow

Solidity handles overflows for basic math operations but not for casting. Unsafe casting may lead to truncation if XXargument is greater than type(uintXX).max.

It is very unlikely to happen in the protocol - for votes with uint192, block.number with uint64 (even with the shift) or PPM and uint32, but consider using a safeCast library.

File: Equity.sol

146:         totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);

161:             voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); // new example anchor is only 21 / 11 = 1 block in the past

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

File: Position.sol

187:             return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

L-3 cooldown can be less than advertised

The documentation specifies that:

Minting is not possible until the initialization period of seven days has passed

This is also echoed in the constructor of Position:

Position.sol
64: start = block.timestamp + initPeriod; // one week time to deny the position
65: cooldown = start;

But in reality, it is possible for initPeriod to be as low as 3 days

Position.sol
53: require(initPeriod >= 3 days);

This can be dangerous in certain cases: For example, imagine a malicious Position is created on a Thursday evening, and valid FPS holders do not pay attention to it until the following Monday, as they believe it is not possible to mint before a 7 day period.

Indicate clearly in documentation that a new position can mint ZCHF after 3 days.

L-4 block.number is not reliable

block.number is not a fully accurate way to measure time, as there can be empty slots, which would make the time between two conscutive blocks greater than expected. Use block.timestamp to measure passage of time.

File: Equity.sol

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

L-5 Adjusting a position does not require to wait for cooldown

The documentation states that:

Once you are the proud owner of a position whose cooldown period has passed, you can start to adjust it

But there is no noCooldown modifier on adjust():

Position.sol
132:     function adjust(uint256 newMinted, uint256 newCollateral, uint256 newPrice) public onlyOwner {
133:         if (newPrice != price){
134:             adjustPrice(newPrice);
135:         }

This means it is possible to increase the price, add collateral and lower the minted amount before the cooldown.

Add a no noCooldown modifier to adjust()

L-6 Wrong comment in notifyChallengeSucceeded()

_size has the same decimals as the collateral token, not 18.

File: Position.sol
326:  * @param _size     size of the collateral bid for (dec 18)

L-7 Denominator can be zero

It is technically possible to launch a challenge with challenge.size == 0 - in the edge case where the position in question has collateralBalance == 0 (meaning the check in Position.notifyChallengeStarted() would pass).

This would make the following line in splitChallenge revert

File: MintingHub.sol

165:             (challenge.bid * splitOffAmount) / challenge.size
             );

There is already a check that the challenge size is greater than the minimum collateral of the position after the split. For better error handling, add a check to ensure the challenge to split has a sufficient size before the split:

File: MintingHub.sol
+            require(challenge.size >= min);
159: Challenge memory copy = Challenge(
160:             challenge.challenger,
161:             challenge.position,
162:             splitOffAmount,
163:             challenge.end,
164:             challenge.bidder,
165:             (challenge.bid * splitOffAmount) / challenge.size
166:         );
167:         challenge.bid -= copy.bid;
168:         challenge.size -= copy.size;
169: 
170:         uint256 min = IPosition(challenge.position).minimumCollateral();
171:         require(challenge.size >= min);

L-8 Lack of validation in splitChallenge()

There is no check on splitOffAmount. If it is set to a value greater than challenge.size, it will make the following line revert with an underflow error:

File: MintingHub.sol
167:         challenge.bid -= copy.bid;

This can happen if two calls to splitChallenge with the same challenge are made in the same block, and the second call processed is trying to split using an amount greater than the challenge size after the first split.

For better error handling, add a check to ensure the split amount is less than the current size:

File: MintingHub.sol
+            require(challenge.size >= splitOffAmount, "split amount greater than size");
159: Challenge memory copy = Challenge(
160:             challenge.challenger,
161:             challenge.position,
162:             splitOffAmount,
163:             challenge.end,
164:             challenge.bidder,
165:             (challenge.bid * splitOffAmount) / challenge.size
166:         );
167:         challenge.bid -= copy.bid;
168:         challenge.size -= copy.size;
169: 
170:         uint256 min = IPosition(challenge.position).minimumCollateral();
171:         require(challenge.size >= min);

L-9 DOS vector in StablecoinBridge.mint()

The bridge only allows minting of ZCHF if its current balance of XCHF does not exceed the limit

File: StablecoinBridge.sol
49:     function mintInternal(address target, uint256 amount) internal {
50:         require(block.timestamp <= horizon, "expired");
51:         require(chf.balanceOf(address(this)) <= limit, "limit");

This brings a DOS vector where an attacker can DOS minting calls by front-running them with a mint() call, hitting the limit so that the victim's call reverts.

As per the tests scripts, the limit would be set to 10M, meaning the attack is currently impossible as the XCHF has a supply of ~3M tokens.

However, this supply is not harcoded and the owner of XCHF (a Gnosis Wallet) can technically mint as many tokens as they want.

As a bridge is supposed to be live for a one year period, we suggest revisiting the limit variable to make it modifiable by trusted entities in case the XCHF supply were to change. You could also simply compute limit as a value proportional to XCHF.totalSupply() using a view function, but that would make minting calls much more expensive due to gas costs.

L-10 Avoid multiplication after division

This can result in unnecessary precision loss

File: 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();//@audit precision loss

If _reservePPM * mintedAmount does not have enough trailing zeros, it will get truncated by 1000000, resulting in precision loss in theoreticalReserve * currentReserve / minterReserve().

Change to:

File: 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();
+209:          return _reservePPM * mintedAmount * currentReserve / minterReserve() / 100000;

L-11 _burn() should check account is not the zero address

As per the function comments:

File: ERC20.sol
195:      * Requirements
196:      *
197:      * - `account` cannot be the zero address.

But there is no such check in the function.

Change to:

File: ERC20.sol
200:     function _burn(address account, uint256 amount) internal virtual {
+            require(account != address(0), "address 0");
201:         _beforeTokenTransfer(account, address(0), amount);
202: 
203:         _totalSupply -= amount;
204:         _balances[account] -= amount;
205:         emit Transfer(account, address(0), amount);
206:     }

Non Critical Issues

NC-1 Constants on the left are better

This is safer in case you forget to put a double equals sign, as the compiler wil throw an error instead of assigning the value to the variable.

Instances (9):

File: ERC20PermitLight.sol

56:             require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");

File: Equity.sol

145:         uint256 lostVotes = from == address(0x0) ? 0 : (anchorTime() - voteAnchor[from]) * amount;

226:         if (owner == delegate){

228:         } else if (owner == address(0x0)){

242:         require(msg.sender == address(zchf), "caller must be zchf");

File: MintingHub.sol

259:         address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;

File: Position.sol

250:         if (token == address(collateral)){

File: StablecoinBridge.sol

76:         if (msg.sender == address(chf)){

78:         } else if (msg.sender == address(zchf)){

NC-2 2**128 - 1 should be re-written as type(uint128).max

Instances (1):

File: Equity.sol

-253:         require(totalSupply() < 2**128, "total supply exceeded");
+253:         require(totalSupply() <= type(uint128).max, "total supply exceeded");

NC-3 Contract layout not following Solidity standard practice

Follow Solidity's style guide for contract layout: Type declarations, State variables, Events, Modifiers, and Functions

In Frankencoin, errors are declared in the middle of the contract. Move them before the constructor. Instances (5):

File: Frankencoin.sol

92: error PeriodTooShort();
93: error FeeTooLow();
94: error AlreadyRegistered();

130: error NotMinter();

159: error TooLate();

NC-4 Order of functions not following Solidity standard practice

Follow Solidity's style guide for function ordering: constructor, receive/fallback, external, public, internal and private.

Instances (24):

File: ERC20.sol

85:     function transfer(address recipient, uint256 amount) public virtual override returns (bool) {

93:     function allowance(address owner, address spender) external view override returns (uint256) {//@audit external defined after public

97:     function allowanceInternal(address owner, address spender) internal view virtual returns (uint256) {

108:     function approve(address spender, uint256 value) external override returns (bool) {//@audit external defined after internal

125:     function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {//@audit external defined after internal

151:     function _transfer(address sender, address recipient, uint256 amount) internal virtual {

162:     function transferAndCall(address recipient, uint256 amount, bytes calldata data) external override returns (bool) {//@audit external defined after internal

File: Equity.sol

112:     function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {

127:     function canRedeem() external view returns (bool){//@audit external defined after internal

135:     function canRedeem(address owner) public view returns (bool) {

144:     function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {

157:     function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){

172:     function anchorTime() internal view returns (uint64){

179:     function votes(address holder) public view returns (uint256) {//@audit public defined after internal

220:     function delegateVoteTo(address delegate) external {

225:     function canVoteFor(address delegate, address owner) internal view returns (bool) {

241:     function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {//@audit external defined after internal

266:     function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {

275:     function redeem(address target, uint256 shares) public returns (uint256) {//@audit public defined after internal

290:     function calculateProceeds(uint256 shares) public view returns (uint256) {//@audit public defined after internal

309:     function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {//@audit public defined after internal

File: Frankencoin.sol

102:    function allowanceInternal(address owner, address spender) internal view override returns (uint256) {//@audit public defined after internal

117:    function minterReserve() public view returns (uint256) {

125:    function registerPosition(address _position) override external {//@audit external defined after public

138:    function equity() public view returns (uint256) {

152:    function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {//@audit external defined after public

204:    function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {

223:    function burnFrom(address payer, uint256 targetTotalBurnAmount, uint32 _reservePPM) external override minterOnly returns (uint256) {//@audit external defined after public

235:    function calculateFreedAmount(uint256 amountExcludingReserve /* 41 */, uint32 reservePPM /* 20% */) public view returns (uint256){

251:    function burnWithReserve(uint256 _amountExcludingReserve, uint32 _reservePPM) external override minterOnly returns (uint256) {//@audit external defined after public

File: MintingHub.sol

124:     function clonePosition(address position, uint256 _initialCollateral, uint256 _initialMint) public validPos(position) returns (address) {

140:     function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {//@audit external defined after public

188:     function minBid(Challenge storage challenge) internal view returns (uint256) {

199:     function bid(uint256 _challengeNumber, uint256 _bidAmountZCHF, uint256 expectedSize) external {//@audit external defined after internal

252:     function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {

281:     function returnPostponedCollateral(address collateral, address target) external {//@audit external defined after public

287:     function returnCollateral(Challenge storage challenge, bool postpone) internal {

314:     function clonePosition(address _existing) external returns (address);//@audit external defined after internal

File: Position.sol

169:     function collateralBalance() internal view returns (uint256){

177:     function mint(address target, uint256 amount) public onlyOwner noChallenge noCooldown alive {//@audit public defined after internal

202:     function restrictMinting(uint256 period) internal {

227:     function repay(uint256 amount) public onlyOwner {//@audit public defined after internal

240:     function notifyRepaidInternal(uint256 amount) internal {

249:     function withdraw(address token, address target, uint256 amount) external onlyOwner {//@audit external defined after internal

286:     function emitUpdate() internal {

292:     function notifyChallengeStarted(uint256 size) external onlyHub {//@audit external defined after internal

File: StablecoinBridge.sol

49:     function mintInternal(address target, uint256 amount) internal {

55:     function burn(uint256 amount) external {//@audit external defined after internal

NC-5 Naming conventions of functions

follow Solidity standard naming conventions: internal and private functions should start with an underscore

Instances (7):

File: Equity.sol

157:     function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){

172:     function anchorTime() internal view returns (uint64){

File: Ownable.sol


45:     function requireOwner(address sender) internal view {

File: Position.sol

202:     function restrictMinting(uint256 period) internal {

240:     function notifyRepaidInternal(uint256 amount) internal {

282:     function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {

286:     function emitUpdate() internal {

NC-6 Inconsistent use of uint

Throughout the contracts, uint256 is used, but uint is used in some instances. For consistency, use only one or the other. This can also be a problem for structured data signing

Instances (4):

File: Equity.sol

91:     event Trade(address who, int amount, uint totPrice, uint newprice); // amount pos or neg for mint or redemption

91:     event Trade(address who, int amount, uint totPrice, uint newprice); // amount pos or neg for mint or redemption

192:         for (uint i=0; i<helpers.length; i++){

196:             for (uint j=i+1; j<helpers.length; j++){

NC-7 Assembly blocks should have comments

Comments clearly explaining what each assembly instruction does will make it easier for users to review the code.

Instances (1):

File: PositionFactory.sol

39:         assembly {