Tadle

Summary

The function CapitalPool::approve() is expecting the token's address as the input param; however, when TokenManager::_transfer() calls into it, the input param used is address(this), which is referring to the address of the contract TokenManager itself. So it is a totally wrong input.

Vulnerability Details

In TokenManager::_transfer() :

And, in CapitalPool::approve():

From above we can easily see the misaligned input params between them.

Impact

First, it will cause direct DOS, because the TokenManager contract does not have an corresponding approve function for the CapitalPool::approve() to call, so the _transfer will always revert, impacting the withdraw function's native token withdrawal.

However, there is a mitigation which is that, an EOA can manually call the CapitalPool::approve() to make the allowance of TokenManager on CapitalPool no longer being 0x0, then this line of buggy code can be bypassed, resolving the DOS issue.

However again, the CapitalPool::approve()'s natspec is saying:

But the access control is not implemented accordingly, so that right now anybody can call it. This could be another bug, a mis-implemented access control, but this bug saved the protocol... Otherwise, the DOS cannot be solved. Due to the complication of this chain of bugs, and one is impacting another, I think it is an M severity issue and needs to be handled with care.

Tools Used

Manual Review

Recommendations

This buggy line of code ICapitalPool(_capitalPoolAddr).approve(address(this)); should be changed to this: ICapitalPool(_capitalPoolAddr).approve(address(_token));