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

Incorrect token address used when approving in the `TokenManager._transfer()` function.

eeyore

Summary

In the TokenManager._transfer() function, address(this) is used when, in fact, the _token address should be used.

Vulnerability Details

The incorrect value is used in the ICapitalPool(_capitalPoolAddr).approve(address(this)); call to the CapitalPool contract. As seen in the CapitalPool.approve() function, it expects a token address, not the TokenManager contract address:

File: CapitalPool.sol
19: /**
20: * @dev Approve token for token manager
21: * @notice only can be called by token manager
22: * @param tokenAddr address of token
23: */
24: function approve(address tokenAddr) external {

Currently, this issue is somewhat mitigated by another error: the lack of access control in the CapitalPool.approve() function. Therefore, the issue is only of Medium severity.

Impact

  • Denial of service.

  • Locking of funds.

Tools Used

Manual review.

Recommendations

Fix the error by using the _token address as a parameter when calling the CapitalPool.approve() function:

File: TokenManager.sol
243: if (
244: _from == _capitalPoolAddr &&
245: IERC20(_token).allowance(_from, address(this)) == 0x0
246: ) {
-247: ICapitalPool(_capitalPoolAddr).approve(address(this));
+247: ICapitalPool(_capitalPoolAddr).approve(_token);
248: }
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.