Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of Handling for Centralized Token Restrictions in joinDAO Allows for Potential Transaction Failures and Gas Loss

cheatc0d33

Summary

The current implementation of MembershipFactory::joinDAO fails to account for USDC's and WBTC's centralized security features including blacklisting and pause mechanisms. Users could face unexpected transaction failures and lost gas fees if their addresses are blacklisted or if the tokens are paused by their administrators.

The contract doesn't handle scenarios where transfers might fail due to administrative actions like blacklisting or pausing. This could lead to users being unable to join DAOs, stuck transactions, and potential loss of gas fees if transactions revert due to these security features being activated.

The issue lies in the assumption that token transfers will only fail due to insufficient balance or allowance. The implementation doesn't account for external administrative controls that could affect token transfers even when balance and allowance checks pass. Additionally, there's no mechanism to handle or notify users about failures due to these token-specific restrictions.

https://github.com/Cyfrin/2024-11-one-world/blob/main/contracts/dao/MembershipFactory.sol#L140

function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
// ... existing checks ...
// These transfers could fail due to blacklisting or pausing
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), owpWallet, platformFees);
IERC20(daos[daoMembershipAddress].currency).transferFrom(_msgSender(), daoMembershipAddress, tierPrice - platformFees);
// State is updated before confirming successful transfers
daos[daoMembershipAddress].tiers[tierIndex].minted += 1;
IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
}

Recommended Fix

Implement proper error handling and state checks for these specific tokens:

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
contract MembershipFactory {
using SafeERC20 for IERC20;
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;
require(tierPrice > 0, "Invalid tier price");
uint256 platformFees = (tierPrice * platformFeeBps) / 10000;
uint256 daoAmount = tierPrice - platformFees;
IERC20 token = IERC20(daos[daoMembershipAddress].currency);
// Pre-transfer checks
require(
token.balanceOf(_msgSender()) >= tierPrice,
"Insufficient token balance"
);
require(
token.allowance(_msgSender(), address(this)) >= tierPrice,
"Insufficient allowance"
);
// Record pre-transfer balances
uint256 preBalancePlatform = token.balanceOf(owpWallet);
uint256 preBalanceDAO = token.balanceOf(daoMembershipAddress);
// Perform transfers using SafeERC20
token.safeTransferFrom(_msgSender(), owpWallet, platformFees);
token.safeTransferFrom(_msgSender(), daoMembershipAddress, daoAmount);
// Verify transfers were successful by checking actual balance changes
require(
token.balanceOf(owpWallet) == preBalancePlatform + platformFees &&
token.balanceOf(daoMembershipAddress) == preBalanceDAO + daoAmount,
"Transfer failed - possible token restriction"
);
// Update state after confirming successful transfers
daos[daoMembershipAddress].tiers[tierIndex].minted += 1;
IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), tierIndex, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, tierIndex);
}
// Add recovery function for emergency situations
function emergencyWithdraw(
address token,
address recipient,
uint256 amount
) external onlyRole(DEFAULT_ADMIN_ROLE) {
IERC20(token).safeTransfer(recipient, amount);
emit EmergencyWithdraw(token, recipient, amount);
}
event EmergencyWithdraw(address token, address recipient, uint256 amount);
}

This improved implementation:

  1. Uses SafeERC20 for safe interactions with all ERC20 tokens

  2. Adds pre-transfer balance and allowance checks

  3. Verifies actual balance changes after transfers

  4. Follows CEI pattern (Checks-Effects-Interactions)

  5. Includes emergency withdrawal functionality for stuck funds

  6. Provides clearer error messages for transfer failures

  7. Maintains proper balance accounting even with token-specific restrictions

These changes make the contract more robust when dealing with USDC, WETH, WBTC, and similar tokens that might have additional transfer restrictions or security features.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope
0xbrivan2 Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

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