Beatland Festival
First Flight #44
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: medium
Invalid

Use call() instead of transfer() in FestivalPass::withdraw function

anuragshingare2005

Use call() instead of transfer() in FestivalPass::withdraw function

Description

  • In FestivalPass::withdraw function, transfer() is used for native ETH withdrawal. The transfer() and send() functions forward a fixed amount of 2300 gas. If target address is an smart contract which contains an recieve() or fallback() function, the withdraw function will fail

function withdraw(address target) external onlyOwner {
@> payable(target).transfer(address(this).balance);
}

Risk

Likelihood: medium

Impact: low

  • Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

  • The use of the deprecated transfer() function for an address will inevitably make the transaction fail.

  • claimer smart contract does not implement a payable function, it can fail

  • claimer smart contract does implement a payable fallback which uses more than 2300 gas unit, it can fail

  • claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300, it can fail

Proof of Concept

  • Add the below test in ./test/FestivalPass.t.sol and to check the output run the command forge test --mt test_withdraw_targetIsContract -vvvv

  • The below test depicts that if target is set to be an contract, can fail the withdraw function

contract EthReciever {
receive() external payable {
while(true) {}
}
}
EthReciever public rec = new EthReciever();
​
function test_withdraw_targetIsContract() public {
// Users buy passes
vm.prank(user1);
festivalPass.buyPass{value: GENERAL_PRICE}(1);
vm.prank(user2);
festivalPass.buyPass{value: VIP_PRICE}(2);
uint256 expectedBalance = GENERAL_PRICE + VIP_PRICE;
assertEq(address(festivalPass).balance, expectedBalance);
// Organizer withdraws
uint256 organizerBalanceBefore = address(rec).balance;
vm.prank(owner);
vm.expectRevert();
festivalPass.withdraw(address(rec));
}

Recommended Mitigation

  • Instead of transfer function for withdrawal, we can use call function

function withdraw(address target) external onlyOwner {
- payable(target).transfer(address(this).balance);
+ uint256 amount = address(this).balance;
+ (bool success,) = target.call{value:amount}("");
+ if(!success){
+ revert();
+ }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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