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

`RapBattle::_battle` function never updates the RapperStats of any `OneShot` NFT resulting in misinformation for `RapperStats.battlesWon` field of every `address`

maziXYZ

Summary

RapBattle::_battle function never updates the RapperStats of any OneShot NFT resulting in misinformation for RapperStats.battlesWon field of every address

Vulnerability Details

In the function execution of RapBattle::_battle, no update is made to increase by 1 the RapperStats.battlesWon field belonging to the winner's address.

RapBattle::_battle code with missing update ⬇

Code
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);
} 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);
}

Impact

Calling OneShot::getRapperStats(uint256 tokenId) with any uint256 value would return a RapperStats with a uint256 battlesWon field that is always ZERO 0.

Proof of Concept: Proof of Code

On a foundry installed machine,

  • setup a foundry test suite contract as below:

Code
contract FindingsTest is Test {
Credibility credTokenContract;
OneShot oneShotTokenContract;
RapBattle rapBattleContract;
Streets streetsContract;
​
address defender;
address challenger;
​
function setUp() public {
credTokenContract = new Credibility();
oneShotTokenContract = new OneShot();
rapBattleContract = new RapBattle(address(oneShotTokenContract), address(credTokenContract));
streetsContract = new Streets(address(oneShotTokenContract), address(credTokenContract));
​
defender = makeAddr("Defender");
challenger = makeAddr("Challenger");
​
// configure CredToken.setStreetsContract
credTokenContract.setStreetsContract(address(streetsContract));
​
// configure OneShot.setStreetsContract
oneShotTokenContract.setStreetsContract(address(streetsContract));
}
​
function readyPlayersForBattle() internal returns(uint256 defenderTokenId, uint256 challengerTokenId) {
vm.startPrank(defender);
defenderTokenId = oneShotTokenContract.getNextTokenId();
oneShotTokenContract.mintRapper();
oneShotTokenContract.approve(address(streetsContract), defenderTokenId);
// let's stake Rapper NFT for defender for 4 days duration
streetsContract.stake(defenderTokenId);
vm.stopPrank();
​
vm.startPrank(challenger);
challengerTokenId = oneShotTokenContract.getNextTokenId();
oneShotTokenContract.mintRapper();
oneShotTokenContract.approve(address(streetsContract), challengerTokenId);
// let's stake Rapper NFT for challenger for 4 days duration
streetsContract.stake(challengerTokenId);
vm.stopPrank();
​
vm.warp(4 days);
​
vm.prank(defender);
streetsContract.unstake(defenderTokenId);
​
vm.prank(challenger);
streetsContract.unstake(challengerTokenId);
}
​
function testRapperBattlesWonNeverUpdates() public {
(uint256 defenderTokenId, uint256 challengerTokenId) = readyPlayersForBattle();
uint256 _credBet = 3;
​
IOneShot.RapperStats memory previousDefenderRapStats = oneShotTokenContract.getRapperStats(challengerTokenId) ;
​
// let's battle ⚔️
​
vm.startPrank(defender);
credTokenContract.approve(address(rapBattleContract), _credBet);
oneShotTokenContract.approve(address(rapBattleContract), defenderTokenId);
rapBattleContract.goOnStageOrBattle(defenderTokenId, _credBet);
vm.stopPrank();
​
uint256 previousDefenderBal = credTokenContract.balanceOf(defender);
​
vm.startPrank(challenger);
credTokenContract.approve(address(rapBattleContract), _credBet);
oneShotTokenContract.approve(address(rapBattleContract), challengerTokenId);
vm.recordLogs();
rapBattleContract.goOnStageOrBattle(challengerTokenId, _credBet);
vm.stopPrank();
​
assertGt(credTokenContract.balanceOf(defender), previousDefenderBal);
​
// check defender's battlesWon
IOneShot.RapperStats memory currentDefenderRapStats = oneShotTokenContract.getRapperStats(defenderTokenId);
​
console.log("currentDefenderRapStats.battlesWon = ", currentDefenderRapStats.battlesWon);
console.log("previousDefenderRapStats.battlesWon ", previousDefenderRapStats.battlesWon);
​
assertGt(currentDefenderRapStats.battlesWon, previousDefenderRapStats.battlesWon);
}
}

  • on your terminal, run the command:

forge test --mt testRapperBattlesWonNeverUpdates -vvvvv

  • results below shows that there's no increment in the IOneShot.RapperStats.battlesWon field of the winner defender

RESULT
Running 1 test for test/FindingsTest.t.sol:FindingsTest
[FAIL. Reason: assertion failed] testRapperBattlesWonNeverUpdates() (gas: 663653)
Logs:
currentDefenderRapStats.battlesWon = 0
previousDefenderRapStats.battlesWon 0
Error: a > b not satisfied [uint]
Value a: 0
Value b: 0

Tools Used

  • Manual Review

Recommendations

  • Refactor the OneShot::onlyStreetContract modifier as follows:

- modifier onlyStreetContract() {
+ modifier onlyStreetOrRapBattleContract() {
- require(msg.sender == address(_streetsContract), "Not the streets contract");
+ require(msg.sender == address(_streetsContract) || msg.sender == address(_rapBattleContract), "Not the streets or rapBattle contract");
_;
}

  • Refactor OneShot contract as follows:

...
+ RapBattle private _rapBattleContract;
​
+ function setRapBattleContract(address rapBattleContract) public onlyOwner {
+ _rapBattleContract = RapBattle(rapBattleContract);
+ }
...

  • Refactor OneShot::updateRapperStats as follows:

function updateRapperStats(
uint256 tokenId,
bool weakKnees,
bool heavyArms,
bool spaghettiSweater,
bool calmAndReady,
uint256 battlesWon
- ) public onlyStreetContract {
+ ) public onlyStreetOrRapBattleContract {
...
Updates

Lead Judging Commences

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

`battlesWon` is never updated

Support

FAQs

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