First Flight #10: One Shot
First Flight #10
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Lack of bet validation allows 0 risk battles

0xKowalski

Summary

Users are able to battle with 0 risk by not setting an allowance, or by not having enough funds to pay out a loss.

Vulnerability Details

When attempting a battle, neither RapBattle:goOnStageOrBattle() or RapBattle:_battle() verify that the user has: a) has set an allowance, and b) has the erc20 tokens needed to cover the battles bet. This allows users to battle for 0 risk by just not setting an allowance (or by simply not having the funds). This only applies when battling as a challenger, as when becoming a defender funds are always transferred into the contract, whereas when challenging, the funds are directly transferred from the challenger to the defender if the challenger loses.

If a user challenges a defender without setting an allowance or without having the funds, there are two potential scenarios:

Challenger wins:

If the challenger wins, the funds are transferred from the contract to the challenger and the defender is dethroned.

else {
// Otherwise, since the challenger never sent us the money, we just give the money in the contract
credToken.transfer(msg.sender, _credBet);
}

Defender wins:

If the defender wins, the transaction fails due to the contract trying to transfer funds directly from the challenger to the defender.

if (random <= defenderRapperSkill) {
// We give them the money the defender deposited, and the challenger's bet
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
}

PoC

This can be verified by adding the following two tests to the test suite:

function testBattleIfChallengerDoesNotHaveEnoughCredits() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
​
vm.startPrank(challenger);
oneShot.approve(address(rapBattle), 1);
cred.approve(address(rapBattle), 3);
cred.transfer(address(rapBattle), cred.balanceOf(challenger)); // get rid of creds
vm.expectRevert();
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
}
function testBattleIfChallengerDoesNotApprove() public twoSkilledRappers {
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
​
vm.startPrank(challenger);
oneShot.approve(address(rapBattle), 1);
vm.expectRevert();
rapBattle.goOnStageOrBattle(1, 3);
vm.stopPrank();
}

Impact

This allows users to battle completely risk free when a defender is on the stage, as if the challenger where to lose the battle, the transaction would simply revert.

Tools Used

Manual review
Foundry test suite

Recommendations

I recommend adding the following two validation checks to the top of the RapBattle:goOnStageOrBattle() method, these checks ensure the user has set up a allowance with greater then or equal to their bet, and it verifies the user has the funds to pay out that bet if they lose:

require(credToken.allowance(msg.sender,address(this)) >= _credBet, "No allowance set");
require(credToken.balanceOf(msg.sender) >= _credBet, "Insufficient balance");
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

missing check for sufficient `_credBet_` approval

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.