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

DOS of all WETH withdrawals

dinkras

Summary

DOS of all WETH withdrawals

Vulnerability Details

When the protocol's users want to withdraw their tokens, they call: TokenManager.withdraw(). When those assets are WETH, the funds are supposed to be transfered via TokenManager._transfer() function. The function is supposed to transfer WETH from the CapitalPool to TokenManager, then TokenManager sends ETH to the user who claimed it.

_transfer(// WETH transferFrom: capitalPool -> address(this)
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr);

In order those transfers to happen, TokenManager needs to have been approved by the CapitalPool contract. However the approval is wrong. The value passed ICapitalPool(_capitalPoolAddr).approve() is the address of the TokenManager, instead of the token that needs to be approved.

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L247

ICapitalPool(_capitalPoolAddr).approve(address(this));// @wrong arg passed for approve

Looking at the impl of CapitalPool.approve()it can be seen that it requires a token address, instead of the address of TokenManager. As a result all withdraw() txs revert due to the wrong approve arg passed.

function approve(address tokenAddr) external {//@audit-issue tokenManager calls this.approve with his address instead of the WETH token!
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
​
(bool success, ) = tokenAddr.call(//@audit-issue 1 : titple CapitalPool:approve should use OZ forceApprove. Tokens like USDT will revert when approve(x !=0) is called and there is an existing allowance
abi.encodeWithSelector(
APPROVE_SELECTOR,// q
tokenManager,
type(uint256).max
)
);

Code snippets

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L160-L166

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L247

Impact

DOS of all WETH withdrawals

Tools Used

Recommendations

Make CapitalPool to approve TokenManager for transfers. Replace the code here:

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L247

- ICapitalPool(_capitalPoolAddr).approve(address(this));// @audit-issue should approve the token not address(this) -> DOS
+ ICapitalPool(_capitalPoolAddr).approve(_token);

Updates

Lead Judging Commences

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