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

Unnecessary Full Balance Transfer in `RaiseBoxFaucet::burnFaucetTokens`

0xzhd

Root + Impact

Description

The burnFaucetTokens function is designed to burn a specified amount of ERC20 tokens (amountToBurn) held by the contract. However, it first transfers the entire contract balance (balanceOf(address(this))) to the owner's address using _transfer, and then burns only the specified amount from the owner's address.

This approach is inefficient because:

  • Transferring more tokens than necessary increases gas costs.

  • It unnecessarily moves tokens to the owner's address, which may not be the intended behavior if the owner does not need to hold these tokens.

A more efficient approach would be to burn the tokens directly from the contract's balance, provided the ERC20 implementation allows it.

Additionally, the function is declared as public instead of external, which is less gas-efficient since it is only called externally.

// @> Root cause in the codebase
function burnFaucetTokens(uint256 amountToBurn) public onlyOwner {
require(amountToBurn <= balanceOf(address(this)), "Faucet Token Balance: Insufficient");
// @> Unnecessary transfer of entire contract balance
_transfer(address(this), msg.sender, balanceOf(address(this)));
_burn(msg.sender, amountToBurn);
}

Risk

Likelihood:

  • The contract's entire token balance is transferred to the owner every time the function is called, even if only a small amount is to be burned.

  • Developers or auditors overlook the unnecessary transfer, assuming it is part of the intended logic.

Impact:

  • Increased gas costs due to transferring the entire contract balance, which could be significant if the balance is large.

  • Unintended transfer of tokens to the owner's address, which may disrupt the faucet's intended token management logic.

Proof of Concept:

  • Mint 1000 tokens to the contract.

  • Check the contract and owner balances before burning.

  • Call burnFaucetTokens to burn 100 tokens.

  • Verify that the entire contract balance is transferred to the owner, and only 100 tokens are burned.

Add the following to the RaiseBoxFaucetTest.t.sol test:

Proof of Code
// Test to demonstrate unnecessary transfer in burnFaucetTokens
function test_UnnecessaryTransferInBurnFaucetTokens() public {
// check owner and contract balances before burning
uint256 contractBalanceBeforeBurn = raiseBoxFaucet.balanceOf(address(raiseBoxFaucet));
uint256 ownerBalanceBeforBurn = raiseBoxFaucet.balanceOf(owner);
​
console.log("Contract Balance Befor Burn:", contractBalanceBeforeBurn);
console.log("Owner Balance Befor Burn:", ownerBalanceBeforBurn);
​
// Call burnFaucetTokens to burn 100 tokens
uint256 amountToBurn = 100 * 10**18;
vm.prank(owner);
raiseBoxFaucet.burnFaucetTokens(amountToBurn);
// Check balances after burning
uint256 contractBalanceAfterBurn = raiseBoxFaucet.balanceOf(address(raiseBoxFaucet));
uint256 ownerBalanceAfterBurn = raiseBoxFaucet.balanceOf(owner);
uint256 expectedOwnerBalance = contractBalanceBeforeBurn - amountToBurn;
​
console.log("Contract Balance After Burn:", contractBalanceAfterBurn);
console.log("Owner Balance After Burn:", ownerBalanceAfterBurn);
​
// Check final balances
assertEq(contractBalanceAfterBurn, 0);
assertEq(ownerBalanceAfterBurn, expectedOwnerBalance);
​
// Logs:
// Contract Balance Befor Burn: 1000000000000000000000000000
// Owner Balance Befor Burn: 0
// Contract Balance After Burn: 0
// Owner Balance After Burn: 999999900000000000000000000
}

Explanation

  • Setup: The test initializes with 1000 tokens (18 decimals, i.e., 1e21) minted to the RaiseBoxFaucet contract, verified by checking the initial contract balance (contractBalanceBeforeBurn). The owner's balance starts at 0.

  • Issue Demonstration: The burnFaucetTokens function is invoked to burn 100 tokens. However, the function first transfers the contract's entire balance (1000 tokens) to the owner before burning only 100 tokens from the owner's address.

  • Result: The test confirms the following:

    • The contract's balance drops to 0 after the transfer.

    • The owner's balance becomes 900 tokens (1000 - 100), indicating that the full contract balance was transferred to the owner, with only 100 tokens subsequently burned.

    • Logs display the balance changes: the contract's initial balance of 1000 tokens (1e21) reduces to 0, and the owner's balance increases to 900 tokens (9e20).

  • Outcome: The test passes, exposing the inefficiency of transferring the entire contract balance to the owner before burning, resulting in an unintended token transfer and increased gas costs.

Recommended Mitigation

Remove the unnecessary transfer of the entire contract balance and burn the tokens directly from the contract's balance. Ensure the ERC20 implementation supports burning from the contract's address.

function burnFaucetTokens(uint256 amountToBurn) public onlyOwner {
require(amountToBurn <= balanceOf(address(this)), "Faucet Token Balance: Insufficient");
- _transfer(address(this), msg.sender, balanceOf(address(this)));
- _burn(msg.sender, amountToBurn);
+ _burn(address(this), 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.