Frankencoin - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Incorrect index access of the addressToWipe array in Equity#restructureCapTable

Proof of Concept

Althought the function is not a common function to call, the protocol should still the function is implemented correctly.

    /**
     * If there is less than 1000 ZCHF in equity left (maybe even negative), the system is at risk
     * and we should allow qualified FPS holders to restructure the system.
     *
     * Example: there was a devastating loss and equity stands at -1'000'000. Most shareholders have lost hope in the
     * Frankencoin system except for a group of small FPS holders who still believes in it and is willing to provide
     * 2'000'000 ZCHF to save it. These brave souls are essentially donating 1'000'000 to the minter reserve and it
     * would be wrong to force them to share the other million with the passive FPS holders. Instead, they will get
     * the possibility to bootstrap the system again owning 100% of all FPS shares.
     */
    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));
        }
    }

in the current implementation, if the user pass in an array addressesToWipe, the only the first addressToWipe (addressesToWipe[0])'s token get burned, while the code expects to burn all the token that each addressesToWipe address hold to in order to restructure cap table

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol change the implementation to

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[i];
		_burn(current, balanceOf(current));
	}
}

Note the line of code, we change from

address current = addressesToWipe[0];

to

address current = addressesToWipe[i];

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L260 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

Impact

During the MintingHub.sol#end function call, if the bidder address is blocklisted, the challanger's fund is locked as well

Proof of Concept

In MintingHub.sol, first, a challanger can create a challange

    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;
    }

the challanger needs to transfer the collateral amount in.

then a bidder can call function bid to settle a challange

   function bid(uint256 _challengeNumber, uint256 _bidAmountZCHF, uint256 expectedSize) external {
        Challenge storage challenge = challenges[_challengeNumber];
        if (block.timestamp >= challenge.end) revert TooLate();
        if (expectedSize != challenge.size) revert UnexpectedSize();
        if (challenge.bid > 0) {
            zchf.transfer(challenge.bidder, challenge.bid); // return old bid
        }
        emit NewBid(_challengeNumber, _bidAmountZCHF, msg.sender);
        // ask position if the bid was high enough to avert the challenge
        if (challenge.position.tryAvertChallenge(challenge.size, _bidAmountZCHF)) {
            // bid was high enough, let bidder buy collateral from challenger
            zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);
            challenge.position.collateral().transfer(msg.sender, challenge.size);
            emit ChallengeAverted(address(challenge.position), _challengeNumber);
            delete challenges[_challengeNumber];
        } else {
            // challenge is not averted, update bid
            if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));
            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;
            }
            zchf.transferFrom(msg.sender, address(this), _bidAmountZCHF);
            challenge.bid = _bidAmountZCHF;
            challenge.bidder = msg.sender;
        }
    }

see this line of code, bidder is set

challenge.bidder = msg.sender;

then later, in the function end, the bidder field come into the use

    function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {
        Challenge storage challenge = challenges[_challengeNumber];
        require(challenge.challenger != address(0x0));
        require(block.timestamp >= challenge.end, "period has not ended");
        // challenge must have been successful, because otherwise it would have immediately ended on placing the winning bid
        returnCollateral(challenge, postponeCollateralReturn);
        // notify the position that will send the collateral to the bidder. If there is no bid, send the collateral to msg.sender
        address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
        (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);
        if (effectiveBid < challenge.bid) {
            // overbid, return excess amount
            IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
        }
        uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;
        uint256 fundsNeeded = reward + repayment;
        if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
        }
        zchf.transfer(challenge.challenger, reward); // pay out the challenger reward
        zchf.burn(repayment, reservePPM); // Repay the challenged part
        emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);
        delete challenges[_challengeNumber];
    }

note the line of code:

// challenge must have been successful, because otherwise it would have immediately ended on placing the winning bid
returnCollateral(challenge, postponeCollateralReturn);
// notify the position that will send the collateral to the bidder. If there is no bid, send the collateral to msg.sender
address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
(address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);

calling notifyChallengeSucceeded on Position.sol contract

    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 even

        notifyRepaidInternal(repayment); // we assume the caller takes care of the actual repayment
        internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update
        return (owner, _bid, volumeZCHF, repayment, reserveContribution);
    }

calling

internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update

calling

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;
}

ok, the code fullfill the code comment,

transfer collateral to the bidder and emit update

but what if the bidder is blocklisted by the collateral token owner after the bidder call bid?

According to

https://github.com/d-xo/weird-erc20#tokens-with-blocklists

Some tokens (e.g. USDC, USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden.

Malicious or compromised token owners can trap funds in a contract by adding the contract address to the blocklist. This could potentially be the result of regulatory action against the contract itself, against a single user of the contract (e.g. a Uniswap LP), or could also be a part of an extortion attempt against users of the blocked contract.

if the bidder is added to blocklist, then the line of code in withdrawInternal will revert and revert the whole transaction

IERC20(collateral).transfer(target, amount);

which actually lock challanger's collateral as well because the call

returnCollateral(challenge, postponeCollateralReturn);

also revert with the transaction.

basically the impact is summarize as:

during the MintingHub.sol#end function call, if the bidder address is blocklisted after the bid call, the challanger's fund is locked as well

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend add the option to let bidder claim the balance instead of sending the balance out

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L293 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L88 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L153

Vulnerability details

Impact

Minter role privilege never expires after the minter role pass the application period

Proof of Concept

The minter role has very high privilege because the minter can mint the frankeinCoin for free.

   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();
      if (minters[_minter] != 0) revert AlreadyRegistered();
      _transfer(msg.sender, address(reserve), _applicationFee);
      minters[_minter] = block.timestamp + _applicationPeriod;
      emit MinterApplied(_minter, _applicationPeriod, _applicationFee, _message);
   }

When minter is suggested, the application period is set

minters[_minter] = block.timestamp + _applicationPeriod;

and if the applicationPeriod is passed, the minter becomes a minter and function isMinter call returns True

function isMinter(address _minter) override public view returns (bool){
  return minters[_minter] != 0 && block.timestamp >= minters[_minter];
}

after the application period, a valid equity vote holder cannot deny a minter as well.

   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);
   }

because of the check block.timestamp > minters[_minter] will revert after passing the application period.

Then if a minter turns rogue, the protocol can be rugged because of the infinite minting privilege.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol set a expiration period of the minter role or at least add method to remove minter role.