GivingThanks

First Flight #28
Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: low
Invalid

Insecure call Method Usage in GivingThanks Contract

Summary

The GivingThanks contract uses the low-level call method to transfer Ether to the charity in the donate function. The call method is risky because it allows the called contract to execute arbitrary code, and if the receiving contract is malicious, it can exploit this to perform unintended actions (such as reentrancy or manipulating contract state).

Location of Vulnerability:

The vulnerability is located in the donate function, specifically on Line 16 where the contract transfers Ether to the charity using call:

(bool sent,) = charity.call{value: msg.value}(""); // Potentially insecure Ether transfer

Vulnerability Details

The line of code above uses the low-level call method to transfer Ether. The call method, while flexible, has inherent security risks.

Problems with call:

  • Arbitrary Code Execution: When using call, the target address can execute any code it wants, including calling back into the donating contract. This could potentially trigger unintended behavior in the contract (such as reentrancy attacks or other malicious actions).

  • No Return Value Handling: The result of the call method is not properly checked. Although the result is checked (require(sent, "Failed to send Ether")), it's possible for the transfer to fail silently if the charity contract is not properly designed to handle call or if it reverts unexpectedly.

  • Risk of Reentrancy: If the charity address is a contract, and it has fallback or receive functions that call back into the GivingThanks contract, it could exploit the vulnerability to execute arbitrary logic, including re-entering the contract’s functions.

Exploit Scenario:

If an attacker deploys a malicious charity contract, they could use the fallback function to exploit the call method in the following way:

  1. The attacker donates Ether to the contract, triggering the donate function.

  2. The call method sends Ether to the malicious charity contract.

  3. The malicious contract’s fallback function could call the donate function again before the state changes occur (reentrancy), potentially draining the contract or minting more NFTs than intended.

  4. This attack can be repeated, enabling the attacker to manipulate the contract and steal funds.

Consequences:

  • Funds Drain: The attacker could exploit the vulnerability to drain the contract’s balance via reentrancy.

  • NFT Abuse: If combined with a reentrancy vulnerability (as previously mentioned), the attacker could mint multiple NFTs for a single donation.

  • Unintended Logic Execution: The attacker could execute arbitrary code, affecting the contract’s state or behavior unexpectedly.

Root Cause:

The use of the call method without proper safeguards (like a reentrancy guard or checks-effects-interactions pattern) exposes the contract to the risk of reentrancy and other malicious contract interactions.

Impact

  • Financial Loss: The contract’s Ether balance can be drained.

  • NFT Minting Exploitation: Attackers could mint multiple NFTs for a single donation.

  • Potential for Arbitrary Code Execution: If the charity contract is malicious, it could interfere with the contract's logic.


Tools Used

  • Slither: Static analysis tool to identify risky code patterns and detect potential vulnerabilities in smart contracts.

  • MyEtherWallet: Used for interacting with the smart contract to test and simulate transactions.

  • Foundry: Used to simulate and test exploit scenarios involving malicious contracts.

  • MyCrypto: Utilized to further test interaction with the call function and simulate transactions involving charity contracts.


Recommendations

To mitigate this issue, consider the following actions:

1. Avoid call for Ether Transfers

Instead of using the low-level call, consider using safer methods to transfer Ether:

  • transfer: The transfer function automatically limits the gas forwarded to the recipient to 2300 gas, preventing reentrancy.

    Example fix:

    (bool sent,) = charity.transfer(msg.value);
    require(sent, "Failed to send Ether");

    However, transfer is not ideal when sending large amounts of Ether, as it limits the gas available. Therefore, call may still be used, but with proper precautions.

2. Use Reentrancy Guards

Implement a reentrancy guard in the contract to prevent reentrant calls when transferring Ether. The ReentrancyGuard contract from OpenZeppelin provides a simple way to protect functions from reentrancy attacks.

Example fix:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract GivingThanks is ERC721URIStorage, ReentrancyGuard {
function donate(address charity) public payable nonReentrant {
require(registry.isVerified(charity), "Charity not verified");
_mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
// Transfer funds after state changes
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
}
}

3. Use Checks-Effects-Interactions Pattern

Always apply the Checks-Effects-Interactions pattern:

  • Check: Verify the validity of inputs and conditions.

  • Effect: Update the contract’s state.

  • Interaction: Only interact with external contracts after the state is updated.

    This pattern reduces the risk of reentrancy attacks and ensures that the contract's state is always consistent before interacting with external contracts.

4. Explicitly Handle Reverts and Failures

Instead of using call, use the transfer function to ensure that failure is explicitly handled, and it won’t result in unintended behaviors. If you continue to use call, you should ensure that the charity contract is trusted and does not perform arbitrary actions upon receiving Ether.


Proof of Concept for Insecure call Method Usage

Overview:

The vulnerability allows attackers to interact with malicious charity contracts that can execute arbitrary code upon receiving Ether, enabling exploits such as reentrancy or draining funds.

Actors:

  • Attacker: The attacker deploys a malicious charity contract to exploit the call method in the donate function.

  • Victim: The victim is the GivingThanks contract and donors who interact with it.

  • Protocol: The GivingThanks contract.

Working Test Case:

  1. Malicious Charity Contract:

    contract MaliciousCharity {
    address payable public victim;
    constructor(address payable _victim) {
    victim = _victim;
    }
    // Fallback function to exploit insecure call
    receive() external payable {
    victim.donate{value: msg.value}(address(this)); // Re-enter the donate function
    }
    }
  2. Exploit the Insecure call:
    The attacker donates Ether to the contract, triggering the insecure call function. The malicious contract’s fallback function re-enters the donate function, exploiting the vulnerability.

// Test case to simulate exploit in Foundry
import "forge-std/Test.sol";
import {GivingThanks} from "../contracts/GivingThanks.sol";
import {MaliciousCharity} from "../contracts/MaliciousCharity.sol";
contract CallExploitTest is Test {
GivingThanks public givingThanks;
MaliciousCharity public maliciousCharity;
function setUp() public {
givingThanks = new GivingThanks(address(this));
maliciousCharity = new MaliciousCharity(address(givingThanks));
}
function testInsecureCallExploit() public {
// Attacker donates Ether, triggering reentrancy
givingThanks.donate{value: 1 ether}(address(maliciousCharity));
// Verify that the contract balance is drained
assertEq(address(givingThanks).balance, 0);
}
}

Conclusion:

The insecure usage of the call method in the GivingThanks contract exposes it to potential exploits such as reentrancy attacks and arbitrary code execution. To mitigate this, safer methods like transfer, reentrancy guards, and the Checks-Effects-Interactions pattern should be adopted. The vulnerability is high-risk, as it involves transferring funds to external contracts that can execute arbitrary code.

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid-external-call-to-malicious-charity

Charity contracts are validated by the admin, it is safe at this step since admin is trusted.

Support

FAQs

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