Rock Paper Scissors

First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: high
Invalid

Any malicious user can burn anyone's token due to unsafe `WinningToken::burnFron` function.

Description

There is mechanism for burning `WinningToken` as the token contract inheriting `ERC20Burnable` contract.
```javascript
@> contract WinningToken is ERC20, ERC20Burnable, Ownable {
```
As the `ERC20Burnable` contract have mechanism for burning tokens from any address which is open to every player or non player.
```javascript
function burnFrom(address account, uint256 value) public virtual {
_spendAllowance(account, _msgSender(), value);
_burn(account, value);
}
```

Impact

Any player or non player can burn anyone's `WinningToken` by calling `burnFrom` function. Results in lost of tokens.

Proof of Concept

Here you can add this function in test file.
```javascript
function test_burnToken() public {
console.log("Starting Balance Of The playerA:", token.balanceOf(playerA));
vm.prank(playerA);
token.transfer(address(game), 1);
assertEq(token.balanceOf(address(game)), 1);
address hacker = makeAddr("hacker");
vm.prank(playerA);
token.approve(hacker, 10);
vm.prank(hacker);
token.burnFrom(playerA, 9);
console.log("Balance of the playerA:", token.balanceOf(playerA));
}
```
result:
```javascript
[PASS] test_sendTokenDirectlyToTheGameContract() (gas: 81669)
Logs:
Starting Balance Of The playerA: 10
Balance of the playerA: 0
Traces:
[86469] RockPaperScissorsTest::test_sendTokenDirectlyToTheGameContract()
├─ [2581] WinningToken::balanceOf(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF]) [staticcall]
│ └─ ← [Return] 10
├─ [0] console::log("Starting Balance Of The playerA:", 10) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::prank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [27738] WinningToken::transfer(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ emit Transfer(from: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], to: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ └─ ← [Return] true
├─ [581] WinningToken::balanceOf(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 1
├─ [0] VM::assertEq(1, 1) [staticcall]
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]
├─ [0] VM::label(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], "hacker")
│ └─ ← [Return]
├─ [0] VM::prank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [24342] WinningToken::approve(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 10)
│ ├─ emit Approval(owner: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], spender: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], value: 10)
│ └─ ← [Return] true
├─ [0] VM::prank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [8343] WinningToken::burnFrom(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], 9)
│ ├─ emit Transfer(from: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], to: 0x0000000000000000000000000000000000000000, value: 9)
│ └─ ← [Return]
├─ [581] WinningToken::balanceOf(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF]) [staticcall]
│ └─ ← [Return] 0
├─ [0] console::log("Balance of the playerA:", 0) [staticcall]
│ └─ ← [Stop]
└─ ← [Return]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.31ms (1.31ms CPU time)
Ran 1 test suite in 27.00ms (7.31ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
```

Recommendations

Protocol should implement function in such a way so that only owner can burn the tokens from the user or not allow anyone to burn tokens icluding owner itself.
update the `WinningToken.sol` as recommended below.
```diff
+ function burnFrom(address account, uint256 value) public override {
+ revert("You can not burn tokens from anyone");
+ }
```
Or make transfer function only callable by owner ot the token.
```diff
+ function transfer(address to, uint256 value) public onlyOwner override returns (bool) {
+ _transfer(msg.sender, to, value);
+ }
```
Updates

Appeal created

m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
gaurangbrdv Submitter
4 months ago
m3dython Lead Judge
4 months ago
m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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