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

Several Vulnerabilities in `upgradeTier()` function in `MembershipFactory.sol

viquetoh

Summary:

The upgradeTier() function in the membership factory has several vulnerabilities that can cause the function to misbehave and potentially lose user funds the vulnerabilities include:

  1. Potential for arithmetic underflow/ overflow.

  2. Lack of balance check before burning tokens.

  3. Ability to upgrade from the lowest tier.
    These vulnerabilities could allow an attacker to burn more tokens than they own or engage in invalid upgrade operations that could lead to loss of user funds.

Vulnerability Details:

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

function upgradeTier(address daoMembershipAddress, uint256 fromTierIndex) external {
require(daos[daoMembershipAddress].daoType == DAOType.SPONSORED, "Upgrade not allowed.");
require(daos[daoMembershipAddress].noOfTiers >= fromTierIndex + 1, "No higher tier available.");
IMembershipERC1155(daoMembershipAddress).burn(_msgSender(), fromTierIndex, 2);
IMembershipERC1155(daoMembershipAddress).mint(_msgSender(), fromTierIndex - 1, 1);
emit UserJoinedDAO(_msgSender(), daoMembershipAddress, fromTierIndex - 1);
}
​

Impact :

  1. Potential for arithmetic underflow/overflow
    The upgradeTier() function subtracts one(-1) from fromTierIndex to determine the new tier index when minting the new token. This could lead to an arithmetic underflow if the fromTierIndex is 0, subtracting 1 from 0 will result in an underflow.

No checks to ensure that, the fromTierIndex is within the valid range of tier indicies.
If an invalid tier index is provided like, value greater than oe equal to TIERMAX the subtraction operation could equally lead to a potential Arithemetic overflow/underflow.

  1. Lack of Balance check before burning tokens:
    The upgradeTier() function does not properly check the user's balance before burning tokens.
    The function calls the burn on the IMembershipERC1155 contract, passing in the fromTierIndex and a fixed value of 2 for the amount to be burned.
    This can lead to a situation where the user does not have enough tokens of fromTierIndex but the function will still attempt to burn two tokens. This will cause the burn() function to revert with an error which is not the expected behavior.

  2. Ability to Upgrade from the lowest tier:
    The upgradeTier() lacks checks to ensure that the fromTierIndex is a valid tier.
    meaning a malicious user could potentially call the function with the lowest tier or otherwise. Subtracting 1 from 0 could lead to an underflow also allowing an upgrade from an invalid tier could also lead to an underflow.

POC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import "forge-std/Test.sol";
import "../contracts/dao/MembershipFactory.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../contracts/dao/interfaces/ICurrencyManager.sol";
import { DAOType, TierConfig, DAOInputConfig, TIER_MAX } from "../contracts/dao/libraries/MembershipDAOStructs.sol";
​
// Mock contract for ICurrencyManager
contract MockCurrencyManager is ICurrencyManager {
function isCurrencyWhitelisted(address) external pure override returns (bool) {
return true;
}
​
function addCurrency(address currency) external override {
}
​
function removeCurrency(address currency) external override {
}
​
function viewCountWhitelistedCurrencies() external view override returns (uint256) {
return 1;
}
​
function viewWhitelistedCurrencies(
uint256 cursor,
uint256 size
) external view override returns (address[] memory, uint256) {
uint256 length = size;
if (length > 1) {
length = 1;
}
​
address[] memory whitelistedCurrencies = new address[]();
whitelistedCurrencies[0] = address(0x1234567890AbcdEF1234567890aBcdef12345678);
​
return (whitelistedCurrencies, cursor + length);
}
}
​
// Mock contract for IMembershipERC1155
contract MockMembershipERC1155 is IMembershipERC1155 {
mapping(address => mapping(uint256 => uint256)) private _balances;
DAOType public daoType;
​
constructor(DAOType _daoType) {
daoType = _daoType;
}
​
function initialize(
string memory,
string memory,
string memory,
address,
address
) external override {
}
​
function mint(address to, uint256 id, uint256 amount) external {
_balances[to][id] += amount;
}
​
function burn(address from, uint256 id, uint256 amount) external {
require(_balances[from][id] >= amount, "ERC1155: burn amount exceeds balance");
_balances[from][id] -= amount;
}
​
function balanceOf(address account, uint256 id) external view returns (uint256) {
return _balances[account][id];
}
}
​
contract UpgradeTierVulnerabilityTest is Test {
MembershipFactory public factory;
MembershipERC1155 public membershipImpl;
MockCurrencyManager public currencyManager;
MockMembershipERC1155 public mockNFT;
address public owpWallet = address(1);
address public attacker = address(2);
address public daoAddress;
​
function setUp() public {
currencyManager = new MockCurrencyManager();
membershipImpl = new MembershipERC1155();
​
factory = new MembershipFactory(
address(currencyManager),
owpWallet,
"baseURI/",
address(membershipImpl)
);
​
TierConfig[] memory tiers = new TierConfig[]();
for (uint i = 0; i < TIER_MAX; i++) {
tiers[i] = TierConfig({
amount: 100,
price: 1 ether,
power: 1 + i,
minted: 0
});
}
​
DAOInputConfig memory daoConfig = DAOInputConfig({
ensname: "test.dao",
currency: address(0),
daoType: DAOType.SPONSORED,
noOfTiers: TIER_MAX,
maxMembers: 700
});
​
vm.startPrank(attacker);
daoAddress = factory.createNewDAOMembership(daoConfig, tiers);
vm.stopPrank();
​
mockNFT = MockMembershipERC1155(daoAddress);
}
​
function testTierUpgradeabiltyVulnurebility() public {
vm.prank(address(factory));
mockNFT.mint(attacker, 2, 1);
​
vm.startPrank(attacker);
vm.expectRevert("ERC1155: burn amount exceeds balance");
factory.upgradeTier(daoAddress, 2);
vm.stopPrank();
​
// 3. Test underflow
vm.startPrank(attacker);
vm.expectRevert("SafeMath: subtraction overflow");
factory.upgradeTier(daoAddress, 0);
vm.stopPrank();
}
​
​

Output generated

[104942] UpgradeTierVulnerabilityTest::testTierUpgradeabiltyVulnurebility()
├─ [0] VM::prank(MembershipFactory: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ └─ ← [Return]
├─ [79166] TransparentUpgradeableProxy::mint(SHA-256: [0x0000000000000000000000000000000000000002], 2, 1)
│ ├─ [74212] MembershipERC1155::mint(SHA-256: [0x0000000000000000000000000000000000000002], 2, 1) [delegatecall]
│ │ ├─ emit TransferSingle(operator: MembershipFactory: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], from: 0x0000000000000000000000000000000000000000, to: SHA-256: [0x0000000000000000000000000000000000000002], id: 2, value: 1)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::startPrank(SHA-256: [0x0000000000000000000000000000000000000002])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf28dceb3: $ERC1155: burn amount exceeds balance)
│ └─ ← [Return]
├─ [7480] MembershipFactory::upgradeTier(TransparentUpgradeableProxy: [0xCB6f5076b5bbae81D7643BfBf57897E8E3FB1db9], 2)
│ ├─ [1999] TransparentUpgradeableProxy::burn(SHA-256: [0x0000000000000000000000000000000000000002], 2, 2)
│ │ ├─ [1538] MembershipERC1155::burn(SHA-256: [0x0000000000000000000000000000000000000002], 2, 2) [delegatecall]
│ │ │ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
│ │ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
│ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
└─ ← [Revert] Error != expected error: panic: arithmetic underflow or overflow (0x11) != ERC1155: burn amount exceeds balance
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.26ms (526.97µs CPU time)
​
Ran 1 test suite in 2.23s (3.26ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/UpgradeTierVulnerability.sol:UpgradeTierVulnerabilityTest
[FAIL: Error != expected error: panic: arithmetic underflow or overflow (0x11) != ERC1155: burn amount exceeds balance] testTierUpgradeabiltyVulnurebility() (gas: 104942)
​
Encountered a total of 1 failing test, 0 tests succeeded
[~/Documents/Competitive_Audit/One_World]
✘  viquetour   main -+
​

Tools Used: Manual review

Recommendations:

  1. Handling Undeflow Scenarios:
    The function should check for underflow scenarios like, such as subtracting 1 from 0 or performing oerations within an invalid tier index.

  2. Implementing Balance Checks:
    The upgradeTier() function should check the user balance for the fromTierIndex if the balance is less than 2 if less than 2, the function should revert with an appropriate error message.

  3. Validate Tier Indicies:
    The function should validate the provided fromTierIndex to ensure that it is within the valid range of tier Indicies. if invalid the function should revert with appropriate error message.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!