Frankencoin - ChrisTina's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L188-L229, https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L252-L276https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L50-L70

Vulnerability details

Impact

Setting the _challengePeriod to 0 when creating a new position prevents a fair auction from taking place. A position can still be challenged, but the challenge expires immediately and nobody can place a bid.
The challenge can subsequently be ended by anyone, which causes the following money flows:
1 The challenger gets a refund of his collateral
2 The challenger obtains a reward for the successful challenge
3 As there is no bidder, the challenge ender obtains part or all of the collateral held by the challenged position contract
4 The owner of the challenged contract keeps their previously minted Frankencoins
5 As the effectiveBid is 0, the Frankencoin contract is notified of a loss, which causes it to cover the outstanding amount from its reserves and, if insufficient, additionally minted ZCHF.

if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
        }

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L267-L271

As no bids can be placed, the challenger can be certain his collateral will be refunded. He can therefore place a very large amount as collateral, causing an extremely large award in ZCHF to be paid out to himself, as demonstrated in the test cases. The reserve is depleted and a large amount of additional tokens minted.

A position can be challenged even if the init period has not passed, so an exploit is immediately possible.
In short, an exploit can be conducted through these steps:
1 Attacker opens a position with a challenge period of 0 2 Attacker immediately challenges their own position
3 Attacker immediately ends the challenge, getting their collateral back and additional Frankencoins paid out
4 Attacker swaps Frankencoins for XCHF through the bridge contract

Proof of Concept

Foundry test showing two possible attack scenarios can be found here: https://gateway.pinata.cloud/ipfs/QmT4fB48ex26KWaAQVjMfm7JM5iHMHRDSzxZ28csy4Ddia
The vulnerability was disclosed to the project owner, who successfully conducted the exploit to secure the funds in the bridge contract
https://etherscan.io/tx/0x80f3947afa1f8441d43732e74058ed23d3fe3fe7f680a9587f3766d1042209a4

Tools Used

Foundry

Recommended Mitigation Steps

• A minimum challenge period should be enforced by the constructor of Position.sol to ensure a fair auction can take place, e.g.

constructor(address _owner, address _hub, address _zchf, address _collateral, 
        uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
        uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
    if (_challengePeriod < 3 days) {
        revert PeriodTooShort(); 
    ...} 

• Disallow challenging a position during the init period and / or when the amount of minted tokens is 0
• Prevent disproportionately large challenger rewards from being paid out
• Review the role of the challenge ender, who obtains part of or all of the challenged position's collateral without having to participate in the auction

Lines of code

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

Vulnerability details

Withdrawing collateral is disallowed during cooldown

    function withdrawCollateral(address target, uint256 amount) public onlyOwner noChallenge noCooldown

source

A cooldown occurs 1 during the init period (source) 2 if a position is denied, cooldown is set to expiration (source) 3 after a challenge is averted (1 day) (source) 4 after the owner increases the liquidation price (3 days) (source) 5 if the owner withdraws collateral below the minimum collateral amount, cooldown is set to expiration (source)

Impact

1 Collateral can not be withdrawn during the init period, e.g. if the position owner decides during the init period not to move forward with the position or loses trust in the system 2 If a position is denied, the collateral can not be withdrawn until the position is expired ** 3 A position has been challenged, but the challenge was averted. In order to avoid further challenges on the position, especially if the price is very volatile, the owner repays the minted amount, but has to wait 1 day to withdraw collateral. 5 If the position owner withdraws collateral below the minimum collateral amount, but not all of it, they can not withdraw the remainder until the position is expired **

** The position might expire far in the future, meaning the owner loses economic access to the collateral for a couple of years, as described in the documentation: expirationSeconds: ... A typical value could be three years. (source)

The position can still be challenged during that time, possibly resulting in a loss of all the provided collateral.

After the position is finally expired, it can be challenged but a challenge can't be averted, meaning all challenges end successfully. If the position is challenged before the owner can withdraw, the collateral will be lost.

Evaluation of impact

2 and 5 high, 1 is probably unintended, 4 is maybe intended, 3 low impact.

1 Freshly created positions can not mint ZCHF until the init period has passed, during which the position can be denied. As no ZCHF have been minted yet, withdrawing collateral does not pose a risk to the system, this should be allowed.

2 There is already a disincentive (non-refundable opening fee) in place for opening a position that will likely get denied.

Note that calling the openPosition method, an opening fee of 1000 ZCHF is automatically deducted. That is why the Frankencoin allowance is necessary. This fee is not returned if the position is denied and goes to the equity holders.

It is not mentioned anywhere that a denied position will also result in the owner not being able to withdraw their collateral.

Proof of Concept

Test cases here: https://gateway.pinata.cloud/ipfs/QmZMLD5TxFUbVfcCaoUYkqaYA9M3GeXkhh2ZvzCJiLuvXV

Tools Used

Foundry

Recommended Mitigation Steps

Allow withdrawing collateral during a cooldown period by removing the noCooldown modifer. If 4 is intended, handle this case separately by decoupling the cooldown (where minting is restricted) and a separate period where withdrawals are restricted.

Quality Assurance / Low Impact

Use most recent compiler version or at least 0.8.4 in pragma statements

Custom errors are used which have been introduced in Solidity 0.8.4, but contracts allow for earlier compiler versions (^0.8.0).

Proof of concept

pragma solidity ^0.8.0;
...
error NotOwner();

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Ownable.sol#L9 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Ownable.sol#L25

Unused imports

MintingHub imports Ownable, but not using / inheriting from it. Can be deleted.

PositionFactory imports IFrankencoin.sol but not using it. Can be deleted.

Equity.sol and StablecoinBridge.sol import IERC677Receiver.sol, but are not implementing it, instead redefining the onTokenTransfer function. Both contracts should inherit from IERC677Receiver.sol and override the onTokenTransfer function.

require() / revert() statements should have descriptive reason strings [NC-22]

Two additional occurences in addition to Automated Findings [NC-22]:

https://github.com/code-423n4/2023-04-frankencoin/blob/f86279e76fd9f810d2a25243012e1be4191a547e/contracts/Equity.sol#L195

https://github.com/code-423n4/2023-04-frankencoin/blob/f86279e76fd9f810d2a25243012e1be4191a547e/contracts/Position.sol#L53

Consider adding custom errors instead of reason strings to save gas.

Prevent division by 0 [LOW‑7][MED-2]

Additional occurence in addition to automated findings:
https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L209

ERC20.sol does not add increaseAllowance / decreaseAllowance functions

The documentation states that

Finally, the non-standard`decreaseAllowance`and`increaseAllowance` functions have been added to mitigate the well-known issues around setting allowances.

(source), but these functions are not present in the ERC20 contract

Impact

When an allowance is increased or decreased, the approved person could take advantage of both the old and the new allowance, as described here

Constructor in Ownable.sol was removed

... in comparison to the standard Open Zeppelin Ownable contract. This is not listed as one of the modifications being made.

As a result, if Ownable.sol was deployed as a standalone contract, the owner would be address(0) because the default value of 0 is assigned to the owner variable, instead of msg.sender being set as owner in the constructor.

The documentation states (source):

By default, the owner account will be the one that deploys the contract. This can later be changed with {transferOwnership}.

, which is incorrect. By default, ownership is assigned to address(0). The inheriting contract has the responsibility of setting the owner variable correctly in the constructor.

Proof of concept

Concept can be tested by deploying the Ownable.sol contract separately, e.g. on Remix. The owner of the contract is address(0) and as a result, ownership can't be transferred.

Recommendations

To avoid problems if Ownable is reused later, this modification should be clearly stated, along with a notice in the documentation that the contract that inherits from Ownable has the responsibility of setting the correct owner in the constructor through the setOwner function.
In the scope of the project this is handled correctly. Only Position.sol inherits from Ownable, which sets the owner in the constructor / initializer.

Constructors can not be virtual, but consider marking the Ownable contract as abstract to further indicate that an implementation is required.

Code repetition

uint256 powX3 = _mulD18(_mulD18(x, x), x);

could make use of the internal _power3 function instead:

uint256 powX3 = _power3(x);

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MathUtil.sol#L24 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MathUtil.sol#L39-L41

Change event parameter

To mint Frankencoin Pool Shares, a user has to call the transferAndCall function in Frankencoin.sol, which calls the onTokenTransferfunction in Equity.sol.
This function emits an event

emit Trade(msg.sender, int(shares), amount, price())

As the msg.sender is always the Frankencoin contract (permissioned function), there is no added value in logging msg.sender. Should be replaced with from, the address buying the pool shares.

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

Named mappings for better readability

Using named mappings introduced in Solidity 0.8.18 sets the name field for the inputs and outputs in the ABI for the mapping’s getter, making it easier for the user to understand what the expected input value and the output value is. For example a user reading from the contract on Etherscan will be presented with the input fields address collateral and address beneficiaryinstead of two address fields.

mapping(address collateral => mapping(address beneficiary => uint256 amount)) public pendingReturns;

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

No impact on deployment cost

Incorrect description of storage variable

IPositionFactory private immutable POSITION_FACTORY; // position contract to clone

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

Incorrect loop

Doesn't burn tokens from all the addresses in the addressesToWipe array as the function intends, only from the first one.

        for (uint256 i = 0; i<addressesToWipe.length; i++){
            address current = addressesToWipe[0];
            _burn(current, balanceOf(current));
        }

should read

        for (uint256 i = 0; i<addressesToWipe.length; i++){
            address current = addressesToWipe[i];
            _burn(current, balanceOf(current));
        }

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