Raisebox Faucet
First Flight #50
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

burnFaucetTokens tranfers all contract's balance

Formal Blocks

Root + Impact

Description

  • Expected behaviour: Transfer the faucet token to the owner to ensure it has enough tokens to burn. Should transfer only the amount to burn.

  • Issue: Transfers all the balance from the contract rather than just the quantity necessary to burn. Then, it causes the contract to run out of funds.

// Root cause in the codebase with @> marks to highlight the relevant section
​
function burnFaucetTokens(uint256 amountToBurn) public onlyOwner {
require(amountToBurn <= balanceOf(address(this)), "Faucet Token Balance: Insufficient");
​
// transfer faucet balance to owner first before burning
// ensures owner has a balance before _burn (owner only function) can be called successfully
@> _transfer(address(this), msg.sender, balanceOf(address(this)));
​
_burn(msg.sender, amountToBurn);
}

Risk

Likelihood: High

  • Every time the burnFaucetTokens function is called, all the funds are transferred to the contract owner.

Impact: High

  • Owners may not be able to burn tokens twice when they burn fewer tokens than the contract's balance, and they do not mint tokens.

  • Users will not be able to claim tokens when owners burn fewer tokens than the contract's balance.

Proof of Concept

  1. testOwnerBurnTwice tests when the owner burns an amount inferior to the INITIAL_SUPPLY_MINTED, but the contract should revert for insufficient balance

  2. testUsersNotAbleToClaimAfterOwnerBurnTokens tests when a user claims tokens after an owner burns an amount inferior to the INITIAL_SUPPLY_MINTED, but the contract should revert for insufficient balance

function testOwnerBurnTwice() public {
vm.startPrank(owner);
raiseBoxFaucet.burnFaucetTokens(100);
raiseBoxFaucet.burnFaucetTokens(100);
vm.stopPrank();
​
assertEq(
raiseBoxFaucet.getFaucetTotalSupply(),
(INITIAL_SUPPLY_MINTED - 200),
"Owner can't burn twice"
);
}
​
function testUsersNotAbleToClaimAfterOwnerBurnTokens() public {
​
vm.startPrank(owner);
raiseBoxFaucet.burnFaucetTokens(100);
vm.stopPrank();
vm.prank(user1);
raiseBoxFaucet.claimFaucetTokens();
​
assertEq(
raiseBoxFaucet.getBalance(user1),
raiseBoxFaucet.faucetDrip()
);
}

Recommended Mitigation

  • Transfer the exact amount the owner wants to burn.

  • Use amountToBurn instead of balanceOf(address(this))

- _transfer(address(this), msg.sender, balanceOf(address(this)));
+ _transfer(address(this), msg.sender, amountToBurn);
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unnecessary and convoluted logic in burnFaucetTokens

Support

FAQs

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