Summary

There is a critical mismatch between the comment and the actual implementation of the withdraw() function in the TokenManager contract. The comment states that the caller must be the owner, but the function can be called by any user when the contract is not paused.

Vulnerability Details

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

The withdraw() function is currently commented like this:

However, the actual implementation allows any user to call the function when the contract is not paused:

The function only has the whenNotPaused modifier and lacks any onlyOwner or similar access control.

Impact

This discrepancy could lead to several issues:

1. Misuse of the contract: Developers or users relying on the comment might assume that only the owner can withdraw, leading to incorrect assumptions about the contract's security model.

2. Unintended fund withdrawal: If the contract was designed with the intention that only the owner should be able to withdraw, the current implementation allows any user to withdraw their balance, which could lead to unexpected behavior or loss of funds.

3. Auditing difficulties: The mismatch between comments and implementation can make the code harder to audit and understand, potentially hiding other vulnerabilities.

Tools Used

• Manual code review

Recommendations

1. Align the implementation with the comment:
   If the intention is that only the owner should be able to withdraw, modify the function to include the onlyOwner modifier:

   function withdraw(

   address _tokenAddress,

   TokenBalanceType _tokenBalanceType

   ) external onlyOwner whenNotPaused {

   // rest of code block

   }

2. Alternatively, update the comment to reflect the actual implementation:
   If the current implementation is correct and any user should be able to withdraw their own balance, update the comment to accurately describe the function's behavior:

   /**

   * @notice Withdraw

   * @dev Callable by any user when not paused

   * @param _tokenAddress Token address

   * @param _tokenBalanceType Token balance type

   */

   function withdraw(

   address _tokenAddress,

   TokenBalanceType _tokenBalanceType

   ) external whenNotPaused {

   // rest of code block

   }

   By addressing this discrepancy, you will significantly improve the contract's clarity, reduce the risk of misuse, and make it easier for developers and auditors to understand the intended behavior of the contract.