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

Missing Approval for ERC20 Token Withdrawals

Tomas0707

Summary

The withdraw function in the TokenManager.sol contract lacks an approve function call for ERC20 tokens when transferring funds from the CapitalPool contract. This omission prevents users from withdrawing their ERC20 tokens, as the required allowance is not set.

Vulnerability Details

When users attempt to withdraw ERC20 tokens through the withdraw function, the code does not include an approve function call to grant the necessary allowance to the CapitalPool contract. As a result, the subsequent transfer fails due to insufficient allowance, leaving users unable to retrieve their ERC20 tokens.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L170-L181

Impact

Users are unable to withdraw their ERC20 tokens from the CapitalPool, leading to a significant disruption in token liquidity and functionality. This could result in a loss of user trust and hinder the protocol's operations.

Proof of Concept

This is the test code.

function test_ask_turbo_chain() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 700);
​
vm.startPrank(user);
vm.expectRevert();
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
​
capitalPool.approve(address(mockUSDCToken));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
vm.stopPrank();
}

The result is like this.

├─ [8858] UpgradeableProxy::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 2)
│ ├─ [8339] TokenManager::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 2) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(4) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0]
│ │ ├─ [2959] MockERC20Token::transferFrom(UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 7000000000000000 [7e15])
│ │ │ └─ ← [Revert] ERC20InsufficientAllowance(0x6891e60906DEBeA401F670D74d01D117a3bEAD39, 0, 7000000000000000 [7e15])
│ │ └─ ← [Revert] TransferFailed()
│ └─ ← [Revert] TransferFailed()
├─ [34067] UpgradeableProxy::approve(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ ├─ [29055] CapitalPool::approve(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
│ │ │ └─ ← [Return] UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0]
│ │ ├─ [3723] MockERC20Token::transferFrom(UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 7000000000000000 [7e15])
│ │ │ ├─ emit Transfer(from: UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 7000000000000000 [7e15])
│ │ │ └─ ← [Return] true
│ │ ├─ emit Withdraw(authority: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, tokenAddress: MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], tokenBalanceType: 2, amount: 7000000000000000 [7e15])
│ │ └─ ← [Stop]
│ └─ ← [Return]

As you can see, withdraw() function call is only working properly after direct call of capitalPool.approve.

Tools Used

Manual code review

Recommendations

Add an approve function call before transferring ERC20 tokens from the CapitalPool contract to ensure that the required allowance is set.

} else {
ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-CapitalPool-approve-missing-access-control

This is at most low severity, even though giving max approvals shouldn't be permisionless, the respective tokenManager address is retrieved from the TadleFactory contract whereby the trusted guardian role is responsible for deploying such contracts as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/factory/TadleFactory.sol#L68). Since the user still has to go through the PreMarkets/DeliveryPlace contracts to perform market actions, this max approval cannot be exploited.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.