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

Profit Distribution Manipulation Through Transfer Order

0xbrett8571

Summary

MembershipERC1155 contract implements a profit-sharing mechanism that calculates user shares based on weighted token holdings across different tiers. The vulnerability lies in the order of operations during token transfers, where profit calculations occur before balance updates, leading to incorrect profit distributions.

Key affected components: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L203-L212

// Profit calculation using stale balances
function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
if (from != address(0)) saveProfit(from);
if (to != address(0)) saveProfit(to);
super._update(from, to, ids, amounts);
}

Vulnerability Details

The profit distribution system in MembershipERC1155 calculates user shares based on token balances at the time of transfer, but processes profit updates before the actual transfer occurs. This creates a critical timing issue where profit calculations use stale balances.

_update function in MembershipERC1155 calls saveProfit before updating token balances through super._update. This means profit calculations use outdated token holdings, leading to incorrect profit distributions.

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L203-L212

// @FOUND - Profit calculation uses pre-transfer balances
function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
if (from != address(0)) saveProfit(from); // Uses old balances
if (to != address(0)) saveProfit(to); // Uses old balances
super._update(from, to, ids, amounts); // Updates balances after profit calculation
}

The profit calculation depends on shareOf: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L169-L177

// @FOUND - Share calculation affected by stale balances during transfers
function shareOf(address account) public view returns (uint256) {
return (balanceOf(account, 0) * 64) +
(balanceOf(account, 1) * 32) +
// ... weights continue
balanceOf(account, 6);
}

The bug occurs because:

  1. Profit calculation in saveProfit() uses shareOf() which depends on current token balances

  2. The profit is saved before the actual transfer updates the balances

  3. This can lead to incorrect profit calculations since the balances used don't reflect the post-transfer state

Proof of Concept

The root cause of the vulnerability lies in the _update function of MembershipERC1155.sol: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L203-L212

function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
if (from != address(0)) saveProfit(from);
if (to != address(0)) saveProfit(to);
super._update(from, to, ids, amounts);
}

The key issues:

  1. The profit saving mechanism executes before the token balances are updated

  2. The saveProfit function calculates shares based on current token balances before the transfer is complete

  3. This order of operations creates a mismatch between actual token ownership and profit distribution

When combined with the profit calculation in shareOf: https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/tokens/MembershipERC1155.sol#L169-L177

function shareOf(address account) public view returns (uint256) {
return (balanceOf(account, 0) * 64) +
(balanceOf(account, 1) * 32) +
(balanceOf(account, 2) * 16) +
(balanceOf(account, 3) * 8) +
(balanceOf(account, 4) * 4) +
(balanceOf(account, 5) * 2) +
balanceOf(account, 6);
}

This creates a scenario where profit shares are locked in based on pre-transfer balances, leading to incorrect profit distribution after transfers occur.

This demonstrate the profit distribution vulnerability!

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import {Test, console2} from "forge-std/Test.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
​
contract ProfitDistributionExploitTest is Test {
MembershipERC1155 implementation;
TransparentUpgradeableProxy proxy;
ProxyAdmin proxyAdmin;
MembershipERC1155 membershipProxy;
IERC20 mockUSDC;
address alice = address(1);
address bob = address(2);
function setUp() public {
mockUSDC = IERC20(deployCode("MockERC20.sol:MockERC20", abi.encode("USDC", "USDC", 6)));
implementation = new MembershipERC1155();
proxyAdmin = new ProxyAdmin(msg.sender);
proxy = new TransparentUpgradeableProxy(
address(implementation),
address(proxyAdmin),
abi.encodeWithSignature(
"initialize(string,string,string,address,address)",
"TestDAO",
"TEST",
"baseURI/",
address(this),
address(mockUSDC)
)
);
membershipProxy = MembershipERC1155(address(proxy));
membershipProxy.grantRole(membershipProxy.OWP_FACTORY_ROLE(), address(this));
deal(address(mockUSDC), alice, 2000e6);
deal(address(mockUSDC), bob, 2000e6);
}
​
function testProfitDistributionExploit() public {
// Initial setup with multiple tokens to amplify the effect
membershipProxy.mint(alice, 0, 5);
vm.startPrank(alice);
mockUSDC.approve(address(membershipProxy), 1000e6);
membershipProxy.sendProfit(1000e6);
uint256 profit1 = membershipProxy.profitOf(alice);
console2.log("\nStep 1 - Initial State:");
console2.log("Alice balance:", membershipProxy.balanceOf(alice, 0));
console2.log("Initial Profit:", profit1);
// Transfer majority of tokens
membershipProxy.safeTransferFrom(alice, bob, 0, 3, "");
console2.log("\nStep 2 - After Transfer:");
console2.log("Alice balance:", membershipProxy.balanceOf(alice, 0));
console2.log("Bob balance:", membershipProxy.balanceOf(bob, 0));
// Second profit distribution
mockUSDC.approve(address(membershipProxy), 1000e6);
membershipProxy.sendProfit(1000e6);
uint256 aliceProfit = membershipProxy.profitOf(alice);
uint256 bobProfit = membershipProxy.profitOf(bob);
console2.log("\nStep 3 - Final Profit State:");
console2.log("Alice profit:", aliceProfit);
console2.log("Bob profit:", bobProfit);
assertTrue(aliceProfit > profit1, "Alice's profit should incorrectly increase");
assertTrue(bobProfit < 1000e6, "Bob's profit should be incorrectly low");
vm.stopPrank();
}
}

Logs:

Ran 1 test for test/ProfitDistributionExploit.sol:ProfitDistributionExploitTest
[PASS] testProfitDistributionExploit() (gas: 311436)
Logs:
Step 1 - Initial State:
Alice balance: 5
Initial Profit: 1000000000
Step 2 - After Transfer:
Alice balance: 2
Bob balance: 3
Step 3 - Final Profit State:
Alice profit: 1400000000
Bob profit: 600000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.61ms (3.58ms CPU time)

The logs show exactly how the profits are being incorrectly distributed:

  1. Initially Alice has 5 tokens and receives 1,000,000,000 in profits

  2. After transferring 3 tokens to Bob (60% of holdings), the profit distribution becomes skewed

  3. In the final state:

    • Alice's profit increased to 1,400,000,000 despite having fewer tokens

    • Bob only received 600,000,000 despite holding more tokens

This shows the vulnerability where profit calculations are not being properly adjusted during token transfers, allowing Alice to retain more profits than she should while Bob receives less than his fair share based on token holdings.

  • Alice's profit increased even after transferring tokens

  • Bob's profit is less than expected for his token share

Impact

  • Users could receive incorrect profit calculations during transfers

  • The lastProfit tracking could be out of sync with actual token ownership

  • This affects all profit distribution mechanisms in the system

  1. Profit distribution becomes misaligned with actual token ownership

  2. Users can receive incorrect profit amounts during claims

  3. The weighted share calculation (shareOf) uses stale balances

  4. Accumulated profits may be incorrectly attributed between sender and receiver

  5. The totalProfit tracking becomes unreliable over multiple transfers

The vulnerability affects the core profit sharing mechanism of the protocol, potentially leading to economic losses for users through incorrect profit distributions.

This issue is particularly dangerous because it impacts all transfer operations and the fundamental profit sharing functionality that token holders rely on.

Recommendations

// Update balances first, then calculate profits using correct token holdings

function _update(
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts
) internal virtual override(ERC1155Upgradeable) {
- if (from != address(0)) saveProfit(from);
- if (to != address(0)) saveProfit(to);
super._update(from, to, ids, amounts); // Update balances first
+ if (from != address(0)) saveProfit(from); // Calculate profits after balance update
+ if (to != address(0)) saveProfit(to);
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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