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

Withdraw of ERC20's that are not WETH always reverts

charlescheerful

Vulnerability Details 🔍 && Impact 📈

Users can deposit value in collateral tokens on the code but can't withdraw it. This is because TokenManager.sol does not call the approve() function in CapitalPool before using _safe_transfer_from() inside the withdraw() function.

Proof Of Concept (PoC) 👨‍💻💻

Paste this test in the PreMarkets.t.sol file, import import "forge-std/console.sol";, and run forge test --mt "test_withdraw" -vvv:

See test code 👁️
function test_withdraw() public {
console.log("-----------------");
console.log("user -> Alice: ", address(user));
console.log("user1 -> Bob: ", address(user1));
console.log("-----------------");
​
console.log("Alice makes an ask maker offer and bob takes it.");
uint256 b_cache = mockUSDCToken.balanceOf(address(user));
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1000 * 1e18,
10_000,
100,
OfferType.Ask,
OfferSettleType.Turbo
)
);
uint256 b_after = mockUSDCToken.balanceOf(address(user));
console.log("1e18 reference: ", 1e18);
console.log("Alice should send 1000$ as collateral: ", b_cache - b_after);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
console.log("Bob takes the whole offer for 1000$");
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 1000);
​
console.log("See Alice balance on tokenManager: ");
uint256 alceBalance =
tokenManager.userTokenBalanceMap(address(user), address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console.log("Alice balance Sales Revenue: ", alceBalance);
console.log("As you will see in foundry logs, it reverts due to lack of allowance.");
console.log("If adding the line in Recomendations section and run test again all works.");
vm.startPrank(user);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
}

Recommendations 🎯

Add this line of code inside the withdraw() function, here :

+ ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
Updates

Lead Judging Commences

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

finding-TokenManager-approve-wrong-address-input

If we consider the correct permissioned implementation for the `approve()` function within `CapitalPool.sol`, this would be a critical severity issue, because the withdrawal of funds will be permanently blocked and must be rescued by the admin via the `Rescuable.sol` contract, given it will always revert [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L36-L38) when attempting to call a non-existent function selector `approve` within the TokenManager contract. The argument up in the air is since the approval function `approve` was made permisionless, the `if` block within the internal `_transfer()` function will never be invoked if somebody beforehand calls approval for the TokenManager for the required token, so the transfer will infact not revert when a withdrawal is invoked. I will leave open for escalation discussions, but based on my first point, I believe high severity is appropriate.

Support

FAQs

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

Give us feedback!