Project

One World
NFTDeFi
15,000 USDC
View results
Submission Details
Severity: high
Invalid

Ignored Return Value of Transfer Function

Summary : The joinDAO function in the MembershipFactory contract ignores the return value of the transferFrom function calls. Specifically, the function calls the transferFrom function twice to transfer platform fees and the tier price minus platform fees from the user to different addresses. However, the function does not check the return value of these function calls, which means that if either of these transfers fails, the function will continue executing as if the transfer was successful.

/// @notice Allows a user to join a DAO by purchasing a membership NFT at a specific tier
/// @param daoMembershipAddress The address of the DAO membership NFT
/// @param tierIndex The index of the tier to join
function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
require(daos[daoMembershipAddress].noOfTiers > tierIndex, "Invalid tier.");
require(daos[daoMembershipAddress].tiers[tierIndex].amount > daos[daoMembershipAddress].tiers[tierIndex].minted, "Tier full.");
uint256 tierPrice = daos[daoMembershipAddress].tiers[tierIndex].price;
uint256 platformFees = (20 * tierPrice) / 100;
daos[daoMembershipAddress].tiers[tierIndex].minted += 1;
//@Q is every data structure getting ipdated?
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), owpWallet, platformFees);
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), daoMembershipAddress, tierPrice - platformFees);
IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, tierIndex);
}

Vulnerability Details : The joinDAO function in the MembershipFactory contract is responsible for transferring platform fees and the tier price minus platform fees from the user to different addresses. However, the function ignores the return value of the transferFrom function calls, which means that if either of these transfers fails, the function will continue executing as if the transfer was successful.

What is the problem?

The problem is that the transferFrom function returns a boolean value indicating whether the transfer was successful or not. If the transfer fails, the function will return false. However, the joinDAO function does not check this return value, which means that it will not detect if the transfer fails.

What are the consequences?

If the transfer of platform fees or the tier price minus platform fees fails, the joinDAO function will continue executing as if the transfer was successful. This can lead to unexpected behavior and potential security vulnerabilities, such as:

  • Incorrect state: The contract's state may become inconsistent if the transfer fails, but the function continues executing as if the transfer was successful.

  • Loss of funds: If the transfer of platform fees or the tier price minus platform fees fails, the user may lose funds that were intended to be transferred.

  • Unauthorized access: If the transfer fails, but the function continues executing, an attacker may be able to access the contract's functionality in an unauthorized way.

Impact : Incorrect State:

If the transfer of platform fees or the tier price minus platform fees fails, but the joinDAO function continues executing as if the transfer was successful, the contract's state may become inconsistent. This means that the contract's internal variables and data structures may not accurately reflect the actual state of the contract.

Consequences of Incorrect State

If the contract's state becomes inconsistent, it can lead to a range of problems, including:

  • Inaccurate balances: The contract may incorrectly report the balance of a user's account, leading to confusion and potential disputes.

  • Incorrect permissions: The contract may grant or deny permissions to users based on an incorrect state, leading to unauthorized access or denied access to legitimate users.

  • Inconsistent behavior: The contract may behave inconsistently, leading to unexpected results and potential security vulnerabilities.

Example of Incorrect State

Suppose the joinDAO function attempts to transfer platform fees from a user's account to the owpWallet address. However, the transfer fails due to insufficient funds in the user's account. If the function continues executing as if the transfer was successful, the contract's state may become inconsistent. For example, the contract may update the user's balance to reflect the transfer, even though the transfer did not actually occur.

Loss of Funds

If the transfer of platform fees or the tier price minus platform fees fails, the user may lose funds that were intended to be transferred. This can occur if the transfer fails due to insufficient funds, a failed transaction, or other reasons.

Consequences of Loss of Funds

If a user loses funds due to a failed transfer, it can lead to a range of problems, including:

  • Financial loss: The user may lose money that they intended to transfer, leading to financial loss and potential disputes.

  • Loss of trust: The user may lose trust in the contract and the platform, leading to a decrease in adoption and usage.

  • Reputation damage: The contract and the platform may suffer reputational damage, leading to a loss of credibility and trust.

Example of Loss of Funds

Suppose the joinDAO function attempts to transfer the tier price minus platform fees from a user's account to the daoMembershipAddress address. However, the transfer fails due to a failed transaction. If the function continues executing as if the transfer was successful, the user may lose the funds that were intended to be transferred.

Unauthorized Access

If the transfer fails, but the joinDAO function continues executing, an attacker may be able to access the contract's functionality in an unauthorized way. This can occur if the contract's state becomes inconsistent, allowing an attacker to exploit the contract's functionality.

Consequences of Unauthorized Access

If an attacker gains unauthorized access to the contract's functionality, it can lead to a range of problems, including:

  • Security vulnerabilities: The attacker may be able to exploit security vulnerabilities in the contract, leading to potential security breaches.

  • Financial loss: The attacker may be able to steal funds or manipulate the contract's state, leading to financial loss and potential disputes.

  • Reputation damage: The contract and the platform may suffer reputational damage, leading to a loss of credibility and trust.

Example of Unauthorized Access

Suppose the joinDAO function attempts to transfer platform fees from a user's account to the owpWallet address. However, the transfer fails due to insufficient funds in the user's account. If the function continues executing as if the transfer was successful, an attacker may be able to access the contract's functionality in an unauthorized way. For example, the attacker may be able to call the joinDAO function again, potentially allowing them to steal funds or manipulate the contract's state.

Proof of Concept Code : Step 1: Vulnerable Contract

First, we'll create a vulnerable contract that ignores the return values of transferFrom.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "@openzeppelin/contracts/access/AccessControl.sol";
contract VulnerableContract is AccessControl {
bytes32 public constant EXTERNAL_CALLER = keccak256("EXTERNAL_CALLER");
address public owpWallet;
constructor(address _owpWallet) {
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(EXTERNAL_CALLER, msg.sender);
owpWallet = _owpWallet;
}
/// @notice Allows a user to join a DAO by purchasing a membership NFT at a specific tier
/// @param daoMembershipAddress The address of the DAO membership NFT
/// @param tierIndex The index of the tier to join
function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
// Simplified checks and state update
// ... (omitted for brevity)
uint256 tierPrice = 1 ether; // Simplified for PoC
uint256 platformFees = (20 * tierPrice) / 100;
// Vulnerable: Ignoring return values
IERC20(daoMembershipAddress).transferFrom(_msgSender(), owpWallet, platformFees);
IERC20(daoMembershipAddress).transferFrom(_msgSender(), address(this), tierPrice - platformFees);
// Mint the membership NFT
// ... (omitted for brevity)
}
}

Step 2: Malicious Contract

Next, we'll create a malicious contract to exploit the vulnerability.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
interface IERC20 {
function transferFrom(address sender, address recipient, uint256 amount) external returns (bool);
}
contract MaliciousContract {
IERC20 public token;
address public vulnerableContract;
constructor(address _token, address _vulnerableContract) {
token = IERC20(_token);
vulnerableContract = _vulnerableContract;
}
function attack() external {
// Approve the vulnerable contract to spend tokens
token.transferFrom(msg.sender, address(this), 1 ether); // Initial transfer to this contract for PoC
token.transferFrom(address(this), vulnerableContract, 1 ether); // Attempt to exploit the vulnerability
}
}

Step 3: Fixing the Vulnerability with SafeERC20

Now, we'll fix the vulnerability by using the SafeERC20 library.

  1. Import and Use SafeERC20:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
contract FixedContract is AccessControl {
using SafeERC20 for IERC20;
bytes32 public constant EXTERNAL_CALLER = keccak256("EXTERNAL_CALLER");
address public owpWallet;
constructor(address _owpWallet) {
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(EXTERNAL_CALLER, msg.sender);
owpWallet = _owpWallet;
}
/// @notice Allows a user to join a DAO by purchasing a membership NFT at a specific tier
/// @param daoMembershipAddress The address of the DAO membership NFT
/// @param tierIndex The index of the tier to join
function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
// Simplified checks and state update
// ... (omitted for brevity)
uint256 tierPrice = 1 ether; // Simplified for PoC
uint256 platformFees = (20 * tierPrice) / 100;
// Secure: Using SafeERC20 to handle transfers
IERC20(daoMembershipAddress).safeTransferFrom(_msgSender(), owpWallet, platformFees);
IERC20(daoMembershipAddress).safeTransferFrom(_msgSender(), address(this), tierPrice - platformFees);
// Mint the membership NFT
// ... (omitted for brevity)
}
}

Summary

In this proof of concept:

  1. Vulnerable Contract: Demonstrates how ignoring transferFrom return values can be risky.

  2. Malicious Contract: Exploits the vulnerability by attempting to transfer tokens without proper checks.

  3. Fixed Contract: Uses OpenZeppelin’s SafeERC20 library to ensure safe token transfers, mitigating the risk of ignored return values.

Using SafeERC20 ensures that the token transfers revert on failure, providing a secure and reliable way to handle ERC20 token transactions. This approach significantly enhances the security of your contract.

Tools Used : VS code

Recommendations : To mitigate the vulnerability of ignoring the return values of the transferFrom function in ERC20 tokens, you should implement checks to ensure these transfers are successful. Using the SafeERC20 library from OpenZeppelin is a great approach to handle these checks securely.

Changes to Avoid the Vulnerability

  1. **Import **SafeERC20:

    • First, import the SafeERC20 library from OpenZeppelin.

    import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
  2. **Using **SafeERC20:

    • Use the using SafeERC20 for IERC20; statement to extend IERC20 with the safe transfer functions provided by the library.

    using SafeERC20 for IERC20;
  3. Update the joinDAO Function:

    • Replace the direct calls to transferFrom with the safe transfer functions safeTransferFrom.

    function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
    require(daos[daoMembershipAddress].noOfTiers > tierIndex, "Invalid tier.");
    require(daos[daoMembershipAddress].tiers[tierIndex].amount > daos[daoMembershipAddress].tiers[tierIndex].minted, "Tier full.");
    uint256 tierPrice = daos[daoMembershipAddress].tiers[tierIndex].price;
    uint256 platformFees = (20 * tierPrice) / 100;
    daos[daoMembershipAddress].tiers[tierIndex].minted += 1;
    // Using SafeERC20 to handle transfers securely
    IERC20(daos[daoMembershipAddress].currency).safeTransferFrom(_msgSender(), owpWallet, platformFees);
    IERC20(daos[daoMembershipAddress].currency).safeTransferFrom(_msgSender(), daoMembershipAddress, tierPrice - platformFees);
    IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
    emit UserJoinedDAO(_msgSender(), daoMembershipAddress, tierIndex);
    }

Summary:

  1. **Import **SafeERC20: Ensures safe handling of ERC20 token transfers.

  2. **Extend IERC20 with **SafeERC20: Provides secure versions of ERC20 functions.

  3. **Use **safeTransferFrom: Replaces direct transferFrom calls with safe methods that revert on failure.

These changes help ensure the security and reliability of your contract by properly handling token transfers and preventing vulnerabilities related to ignored return values.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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