Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

The `TokenManager.sol` lacks protection against re-initialization

spomaria

Summary

Upgradeable contracts use the initialize function to initialize state variables instead of the constructor in traditional contracts. The TokenManager.sol contract uses the initialize function to initialize the TokenManagerStorage::wrappedNativeToken variable.

Vulnerability Details

The TokenManager::initialize function has no protection against re-initialization

function initialize(address _wrappedNativeToken) external onlyOwner {
wrappedNativeToken = _wrappedNativeToken;
}

Thus, the contract can be re-intialized changing the initial values of the state variables. In fact, the contract can be re-initialized so that the wrappedNativeToken is assigned address(0) as the TokenManager::initialize also lacks address zero check.

Impact

When TokenManager.sol is re-initialized using the TokenManager::initialize function, it results in different TokenManagerStorage::wrappedNativeToken address for users who interact with the protocol before and after the re-initialization of the contract.

Tools Used

Manual Review

Foundry

Proof of Concept:

PoC Place the following code into `PreMarkets.t.sol`.
function test_TokenManager_Can_ReInitialize() public {
// Check whitelisted tokens
address nativeTokenBefore = tokenManager.wrappedNativeToken();
WETH9 wethNine = new WETH9();
​
// re-initialize the TokenManager contract
vm.prank(user1);
tokenManager.initialize(address(wethNine));
// using address(0) works too as there is no zero address check
// tokenManager.initialize(address(0));
​
address nativeTokenAfter = tokenManager.wrappedNativeToken();
assertEq(nativeTokenBefore, nativeTokenAfter);
}

Run: forge test --match-test test_TokenManager_Can_ReInitialize

Output:

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[FAIL. Reason: assertion failed] test_TokenManager_Can_ReInitialize() (gas: 553226)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.09ms (166.34µs CPU time)

Thus the address of the wrappedNativeToken has changed after re-initialization.

Recommendations

  1. Consider protecting the TokenManager::initialize function against re-initialization by using the initializer modifier from openzeppelin.

  2. Consider adding the zero address check to the TokenManager::initialize function

+ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
.
.
.
contract TokenManager is
TokenManagerStorage,
Rescuable,
Related,
- ITokenManager
+ ITokenManager,
+ Initializable
{
.
.
.
- function initialize(address _wrappedNativeToken) external onlyOwner {
+ function initialize(address _wrappedNativeToken) external onlyOwner initializer {
+ if(_wrappedNativeToken == address(0)) {
+ revert();
+ }
​
wrappedNativeToken = _wrappedNativeToken;
}

This way, the TokenManager.sol contract can only be initiallized once and will carry out the zero address check.

Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-Admin-Errors-Malicious

The following issues and its duplicates are invalid as admin errors/input validation/malicious intents are1 generally considered invalid based on [codehawks guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). If they deploy/set inputs of the contracts appropriately, there will be no issue. Additionally admins are trusted as noted in READ.ME they can break certain assumption of the code based on their actions, and

Support

FAQs

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