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

`MembershipFactory::joinDAO` function uses unsafe ERC20 `transferFrom` instead of `safeTransferFrom`

4lifemen

Summary

The MembershipFactory::joinDAO() function uses transferFrom instead of `safeTransferFrom` and without checking return values, which could lead to silent failures with non-standard ERC20 tokens that might be added in the future through the currency manager.

Vulnerability Details

Some ERC20 tokens (like USDT) don't revert on failure but return false instead. Since the contract uses transferFrom without checking the return value:

If transfer fails:

  • The token returns false instead of reverting and return value is not checked

  • The membership NFT is still minted

// Platform fee transfer without return value check
@> IERC20(daos[daoMembershipAddress].currency).transferFrom(
_msgSender(),
owpWallet,
platformFees
);
​
// DAO payment without return value check
@> IERC20(daos[daoMembershipAddress].currency).transferFrom(
_msgSender(),
daoMembershipAddress,
tierPrice - platformFees
);

Function affected

Proof of concept

function test_userGetTokenEvenTransferFailed() public {
// 1. Setup DAO with single tier
DAOInputConfig memory config = DAOInputConfig({
ensname: "test",
daoType: DAOType.PUBLIC,
currency: address(currency),
maxMembers: 100,
noOfTiers: 1
});
​
TierConfig[] memory tiers = new TierConfig[]();
tiers[0] = TierConfig({price: 1e18, amount: 10, power: 1, minted: 0});
​
vm.startPrank(alice);
address daoAddress = factory.createNewDAOMembership(config, tiers);
vm.stopPrank();
​
vm.startPrank(bob);
// not revert if bob's balance isn't sufficient
factory.joinDAO(daoAddress, 0);
uint256 balance = MembershipERC1155(daoAddress).balanceOf(bob, 0);
console2.log("Bob's balance", balance);
vm.stopPrank();
}

Log shows Bob still gets the share even if transfers failed

Logs:
Bob's balance 1

Impact

The protocol currently only supports WBTC, WETH, and USDC - all of which properly revert on failed transfers, making this temporarily safe. However, if the protocol adds support for additional tokens through the currency manager (e.g., USDT, which is a commonly requested token), silent failures would lead to users receiving membership NFTs without successful payment.

Tools Used

  • Manual review

  • Foundry

Recommendations

Import and use OpenZeppelin's SafeERC20 library to handle ERC20 transfers safely:

+ using SafeERC20 for IERC20;
​
function joinDAO(address daoMembershipAddress, uint256 tierIndex) external {
...
- IERC20(daos[daoMembershipAddress].currency).transferFrom(
_msgSender(),
owpWallet,
platformFees
);
+ IERC20(daos[daoMembershipAddress].currency).safeTransferFrom(
_msgSender(),
owpWallet,
platformFees
);
​
// @notes transfer from msgsender to Dao address
// @audit use SafeTransferFrom
- IERC20(daos[daoMembershipAddress].currency).transferFrom(
_msgSender(),
daoMembershipAddress,
tierPrice - platformFees
);
+ IERC20(daos[daoMembershipAddress].currency).safeTransferFrom(
_msgSender(),
daoMembershipAddress,
tierPrice - platformFees
);
...
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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