Frankencoin - __141345__'s results

Lines of code

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

Vulnerability details

Impact

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.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Disallow transferring position ownership to addr(0)

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L140-L148 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L292-L296 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L252-L276 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L329-L354

Vulnerability details

Impact

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. If there is no limitation to launch challenges, attackers can abuse the challenge rule to steal collateral and reserve, innocent bidders and the protocol will lose fund. There might also some side effects that innocent challengers and bidders collateral and bid fund could not be returned and get stuck.

Proof of Concept

Imagine the following, a position with 20,000 zchf collateral, minimumCollateral for challenge is 1,000 zchf. The attacker can launch 20 challenges with minimumCollateral. But at the same time, there is another challenge with exactly 20,000 challenge size. At the end of the auction, the 20 challenges of the attacker have no bids, and the other 1 has a bid less than the position price(success challenge).

Right at the moment of challenge.end, the attacker will call MintingHub.sol#end() for all the 20 challenges. Even if the bidder of 20,000 challenge tries to call end() also, the attacker can frontrun to make sure the 20 rogue transactions are processed first. The 20 challenges with 0 bids will be considered succeed but with a loss. As a result, the attacker will receive collaterals of 1,000 zchf 20 times plus challenger reward, but paying nothing.

For challenges with no bids, the msg.sender of end() will be the recipient, in this case it will be the attacker. When calls function notifyChallengeSucceeded(), the _bidder(recipient) will be the attacker, _bid will be 0, _size will be 1,000 zchf (minimumCollateral). For each end() call, the attacker will get the collateral of minimum amount 1,000 zchf in line 352 of Position.sol. Since there is no bid fund, in line 270 of MintingHub.sol, notifyLoss() will be called, leaving the protocol to take the loss.

File: contracts/MintingHub.sol
252:     function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {

258:         // notify the position that will send the collateral to the bidder. If there is no bid, send the collateral to msg.sender
259:         address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
260:         (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);

269:         } else if (effectiveBid < fundsNeeded){
270:             zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything

File: contracts/Position.sol
329:     function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {

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

At last, the collateral balance of the position will be drained, the innocent bidder with the 20,000 challenge can get nothing. Even if the innocent bidder's tx get included before the collateral is drained, it is probably only a portion of the position collateral can be obtained, the amount will be scaled by the collateral balance left as below:

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

There could be one more side effect to lock the challenger's collateral and bidder's return fund. Since the position collateral could be drained, the withdrawal amount is possible to be 0, making the above call to fail, because 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. Because the returnCollateral() call is inside function end(), the revert of notifyChallengeSucceeded() could prevent the return of collateral and bid fund.

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

The problem with too many concurrent challenges is that, inevitably it is likely to have some challenges with lower bid price than other challenges. Those bidders will have incentives to frontrun the end() when the challenges reach the deadline, since they pay less or even 0, but with the possibility to get reward or collateral, effectively they are stealing the fund from other challenge bidders. Because of this rule, malicious users will tend to abuse it, creating many challenges, and frontrun those with no bids to steal collateral and challenger reward from the protocol. The side effects could harm the innocent bidders with higher prices, they will fail in the auction to get the fund they deserve. The other challengers' collateral could be locked if the collateral is drained and the collateral erc20 revert on 0 amount transfer. And the protocol will lose fund for those rogue challenges.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• Check for the total challenge amount, disallow the total challenge to be more than the position collateral .

• Separate the return of collateral and bid fund from end(), provide the option for the challenger and bidder to pull the fund, in case the other function calls fail in end(). Making the fund return not dependent on other factors, and the system could be more robust. Just like the returnPostponedCollateral() in MintingHub.sol.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L140-L148 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L292-L296 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L169-L171 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L252-L276 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L329-L354

Vulnerability details

Impact

There is no check for the condition for the position when launching a challenge. An attacker can launch challenge on "dead" positions, such as closed positions, or positions with 0 minted amount, with or without collateral. The collateral balance could be manipulated by directly transfer collateral into the position to manipulating the function Position.sol#collateralBalance(). The attacker can game the system to steal fund as challenger reward, until the protocol fund is drained.

Proof of Concept

There is no check for the condition of the position, or the challenge size upper limit to create a challenge, so a closed position with 0 collateral can be chosen to launch a challenge with any big size:

File: contracts/MintingHub.sol
140:     function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
141:         IPosition position = IPosition(_positionAddr);
142:         IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
143:         uint256 pos = challenges.length;
144:         challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
145:         position.notifyChallengeStarted(_collateralAmount);
146:         emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
147:         return pos;
148:     }

File: contracts/Position.sol
292:     function notifyChallengeStarted(uint256 size) external onlyHub {
293:         // require minimum size, note that collateral balance can be below minimum if it was partially challenged before
294:         if (size < minimumCollateral && size < collateralBalance()) revert ChallengeTooSmall();
295:         challengedAmount += size;
296:     }

The collateralBalance() calls the collateral token's balanceOf() method to get the balance. For close position with 0 balance collateral, if directly transfer 10,000 zchf worth collateral into the position, collateralBalance() will return 10,000 zchf.

File: contracts/Position.sol
169:     function collateralBalance() internal view returns (uint256){
170:         return IERC20(collateral).balanceOf(address(this));
171:     }

Assuming the reward ratio is 5%, the attacker can do the following:

1. find some closed position with 0 collateral balance.
2. call intingHub.sol#launchChallenge() with 10,000 zchf worth for _collateralAmount.
3. when the time reaches challenge.end, the attacker will transfer 10,000 zchf directly to the challenged position.
4. call MintingHub.sol#end(),
   • line 257, the attacker will receive the collateral returned for challenger.
   • line 259, since there is no bid, the attacker as the msg.sender will be the recipient.
   • Position.sol#notifyChallengeSucceeded():
     • line 331, colBal will be 10,000 zchf due to the direct transfer.
     • _bid will be 0 since there is no bid.
     • _size will be worth 10,000 zchf since it is the amount launched with.
     • line 347 volumeZCHF will be 10,000.
     • line 349 repayment will be 0 because minted is 0.
   • back in MintngHub.sol, line 265, reward will be 10,000 * 5% = 500 zchf.
   • the protocol will take the loss by notifyLoss() and the attacker will get the reward 500 zchf in line 272.

File: contracts/MintingHub.sol
252:     function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {

255:         require(block.timestamp >= challenge.end, "period has not ended");

257:         returnCollateral(challenge, postponeCollateralReturn);
258:         // notify the position that will send the collateral to the bidder. If there is no bid, send the collateral to msg.sender
259:         address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
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);
269:         } else if (effectiveBid < fundsNeeded){
270:             zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
271:         }
272:         zchf.transfer(challenge.challenger, reward); // pay out the challenger reward
273:         zchf.burn(repayment, reservePPM); // Repay the challenged part
274:         emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);
275:         delete challenges[_challengeNumber];
276:     }


File: contracts/Position.sol
329:     function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {

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
353:         return (owner, _bid, volumeZCHF, repayment, reserveContribution);
353:         return (owner, _bid, volumeZCHF, repayment, reserveContribution);
354:     }

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 above steps can be written into a smart contract and executed right at the moment of challenge ending. This attack can be repeated, and the challenge size for the base amount for award can be set arbitrarily big. So that the protocol fund could be drained this way.

This vector also applies to positions with 0 minted amount, with or without collateral. The collateral balance difference could be manipulated by direct transfer as mentioned above.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• disallow creating challenge for closed positions.
• put upper limit for challenge size, such as 200% of the minted amount.
• use some inner accounting for the collateral balance, such as record the balance difference for each collateral transfer, and then update the inner accounting. As the same time, add some admin method to retrieve excess collateral more than the inner accounting, which makes the balance manipulation fail.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L124-L132 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L97-L101

Vulnerability details

Impact

The limit variable in Position.sol constrains the maximum amount a position owner can mint. But anyone can freely clone this position and reduce the limit of the original position. Even some griefer can clone some specific position multiple times on purpose to make it useless, because the minting availability is largely affected. As a result, the original position owner could lose the limit until very little to mint.

Since the reduction in limit is exponentially decay, popular positions are more likely to be affected by this issue. Even if the clone users do not intend to grief the owner, the rapid decay speed could potentially harm the original position owner.

The purpose of the position creation is to let the owner mint zchf, however, uncontrolled clone mechanism can make the position owner fail to mint at will. The expected contract minting operation may not be achieved.

Proof of Concept

Anyone can call MintingHub.sol#clonePosition() to clone an existing position. And during the creation of the clone, reduceLimitForClone() will be called, the limit of the original clone is at least cut in half. At the end, for the cloned position, the new owner is initialized to the caller(msg.sender), and limit to be half of the original one.

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

130:         IPosition(pos).initializeClone(msg.sender, existing.price(), limit, _initialCollateral, _initialMint);
131:         return address(pos);
132:     }

File: contracts/Position.sol
097:     function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
098:         uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
099:         limit -= reduction + _minimum;
100:         return reduction + _minimum;
101:     }

Since every time the limit is cut in half, after several clone, the limit will decay exponentially, until reaching 0, making the original position useless.

When the limit is approaching, the owner cannot mint new zchf:

193:     function mintInternal(address target, uint256 amount, uint256 collateral_) internal {
194:         if (minted + amount > limit) revert LimitExceeded();

Those reduced limit amount is redistributed to the clone positions, but only available to the new owner to mint, enforced by the onlyOwner modifier, the original position owner can do nothing for the cloned position:

File: contracts/Position.sol
177:     function mint(address target, uint256 amount) public onlyOwner noChallenge noCooldown alive {
178:        mintInternal(target, amount, collateralBalance());
179:     }

Tools Used

Manual analysis.

Recommended Mitigation Steps

• give the original position owner more control over the clone. Maybe let original owner have some whitelist to call clone, or allow the original owner to mint from cloned positions.
• the reduced amount of the limit can be more flexible, such as specifying fixed reduced amount, instead of cut in half with exponential decay.

Lines of code

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#L380-L383

Vulnerability details

Impact

With any ongoing challenge, the position will be inoperable for mint/withdrawCollateral/clone/adjustPrice. A griefer can abuse the challenge rule to make the position non functional.

The impact of this issue varies depends on the specific parameters of the position. For those with shorter challengePeriod and bigger minCollateral, the impacts could be less harmful, but for some assets with long challengePeriod and small minCollateral, the position could stop working for relatively long time, it is also easier to abuse the challenge, the position owners could suffer more.

Proof of Concept

The noChallenge modifier will prevent the following functions calls:

File: contracts/Position.sol
380:     modifier noChallenge() {
381:         if (challengedAmount > 0) revert Challenged();
382:         _;
383:     }

97:     function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {

159:     function adjustPrice(uint256 newPrice) public onlyOwner noChallenge {

177:     function mint(address target, uint256 amount) public onlyOwner noChallenge noCooldown alive {

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

Although the auction rule is designed that the position owner can not manipulate the auction. The challenger can grief the position at low cost. With minimumCollateral, anyone can DoS the above functions. The challenger can bid() without lose anything.

• if the bid price ends up below the position price, the challenge succeeds, the challenger can get reward plus buying the collateral at lower price. Or the challenger can continue bid() to extend the auction, just making the position non-functional.
• until the bid price ends up above the position price, the bid fund just returns to the challenger, so as the collateral.

Every bid() can prolong the challenge, until high enough to avert, and the griefer could receive the bid fund and collateral.

File: contracts/MintingHub.sol
199:     function bid(uint256 _challengeNumber, uint256 _bidAmountZCHF, uint256 expectedSize) external {

217:             uint256 earliestEnd = block.timestamp + 30 minutes;
218:             if (earliestEnd >= challenge.end) {
219:                 // bump remaining time like ebay does when last minute bids come in
220:                 // An attacker trying to postpone the challenge forever must increase the bid by 0.5%
221:                 // every 30 minutes, or double it every three days, making the attack hard to sustain
222:                 // for a prolonged period of time.
223:                 challenge.end = earliestEnd;
224:             }

Until the bid the high enough to avert the challenge, the above can be repeated over and over, so that any position might be griefed to lose most functionality.

Tools Used

Manual analysis.

Recommended Mitigation Steps

There does not seem easy way to prevent the malicious challenge. Maybe put constrain on the challengePeriod and minCollateral. Upper limit on challengePeriod can prevent the position being affect too long. Larger minCollateral can make the cost for challenge higher. Delayed transfer of collateral after the auction might also help to increase the cost for malicious challenge, if the collateral can not be returned after several days, it will be harder to repeat launching challenges.

Lines of code

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

Vulnerability details

Impact

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.

Tools Used

Manual analysis.

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 from end(), provide the option for the challenger and bidder to pull the fund, then in the case that the other transfer or external calls fail in end(), 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 .

use enumerable set to reduce complexity to O(N)

Currently the votes() function uses nested for loop to check for duplicate in helper[], with O(N^2) complexity. However, OZ enumerable set lib can be used to reduce the complexity to O(N) for the entire function and O(1) for the inner check in line 196-198.

File: contracts/Equity.sol
190:     function votes(address sender, address[] calldata helpers) public view returns (uint256) {
191:         uint256 _votes = votes(sender);
192:         for (uint i=0; i<helpers.length; i++){
193:             address current = helpers[i];
194:             require(current != sender);
195:             require(canVoteFor(sender, current));
196:             for (uint j=i+1; j<helpers.length; j++){
197:                 require(current != helpers[j]); // ensure helper unique
198:             }
199:             _votes += votes(current);
200:         }
201:         return _votes;
202:     }

_transfer() can be moved after if block

If the allowance is not enough, the whole function will revert. Hence the _transfer() can be moved to the end, in case the allowance is not enough, the gas used for _transfer() can be saved.

File: contracts/ERC20.sol
125:     function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
126:         _transfer(sender, recipient, amount);
127:         uint256 currentAllowance = allowanceInternal(sender, msg.sender);
128:         if (currentAllowance < INFINITY){
129:             // Only decrease the allowance if it was not set to 'infinite'
130:             // Documented in /doc/infiniteallowance.md
131:             if (currentAllowance < amount) revert ERC20InsufficientAllowance(sender, currentAllowance, amount);
132:             _approve(sender, msg.sender, currentAllowance - amount);
133:         }
134:         return true;
135:     }