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

The `RapBattle::goOnStageOrBattle` function is missing check for sufficient `_credBet_` in possesion

farismaulana

Description: only the defender require to sent _credBet amount of CRED to the contract and combined with the missing check, challenger with 0 CRED can win the bet or revert when lose

Impact: defender role can lose their bet but challenger can have nothing to lose, making it unfair

Proof of Concept:

  1. Alice mint _tokenId = 0

  2. Alice stake using Streets::stake for 1 days and got 1 CRED

  3. Alice call goOnStageOrBattle with _credBet set to 1 and got the defender role

  4. Slim Shady mint _tokenId = 1

  5. Slim Shady call goOnStageOrBattle function and got the challenger role

  6. The scenario is:

    1. Slim Shady lose -> goOnStageOrBattle is reverted because insufficient balance

    2. Slim Shady won -> Slim Shady got 1 CRED, Alice lose 1 CRED

Proof of Code

add this to the OneShotTest.t.sol:

function testBattleWithInsufficientCred(uint256 randomBlock) public {
// Alice the Defender
vm.startPrank(user);
oneShot.mintRapper(); // _tokenId = 0
oneShot.approve(address(streets), 0);
streets.stake(0);
​
vm.warp(1 days + 1);
streets.unstake(0);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 1);
rapBattle.goOnStageOrBattle(0, 1);
vm.stopPrank();
​
// Slim Shady the Challenger
vm.startPrank(challenger);
oneShot.mintRapper(); // _tokenId = 1
oneShot.approve(address(rapBattle), 1);
cred.approve(address(rapBattle), 1);
​
vm.roll(randomBlock);
vm.recordLogs();
rapBattle.goOnStageOrBattle(1, 1);
vm.stopPrank();
​
Vm.Log[] memory entries = vm.getRecordedLogs();
// Convert the event bytes32 objects -> address
address winner = address(uint160(uint256(entries[0].topics[2])));
if (winner == address(challenger)) {
console.log("[*] Slim Shady is winning the bet!");
} else console.log("[!] transaction revert");
assert(cred.balanceOf(winner) == 1);
}

Recommended Mitigation:
Make sure that RapBattle::goOnStageOrBattle check if the msg.sender actually have sufficient CRED balance equal or greater than _credBet.
Uncomment the line of code where CRED is transferred from challenger to the contract, and adjust the logic for transfering totalPrize to winner.

RapBattle::goOnStageOrBattle

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
+ require(credToken.balanceOf(msg.sender) >= _credBet, "Insufficient CRED balance");
if (defender == address(0)) {
defender = msg.sender;
.
.
.
} else {
- // credToken.transferFrom(msg.sender, address(this), _credBet);
+ credToken.transferFrom(msg.sender, address(this), _credBet);
_battle(_tokenId, _credBet);
}

RapBattle::_battle

if (random <= defenderRapperSkill) {
- // We give them the money the defender deposited, and the challenger's bet
- credToken.transfer(_defender, defenderBet);
+ credToken.transfer(_defender, totalPrize);
- credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
- // Otherwise, since the challenger never sent us the money, we just give the money in the contract
- credToken.transfer(msg.sender, _credBet);
+ credToken.transfer(msg.sender, totalPrize);
}
totalPrize = 0;
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.