Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Misplacement of storage in logic contract instead of proxy leads to complete state loss upon upgrades

0x5chn0uf

Summary

The current implementation incorrectly places the storage variables in the logic contract (the core/ files, such as the TokenManager.sol) and its parent contracts (the storage/ files, such as TokenManagerStorage) instead of in the proxy contract.

This is a fundamental misunderstanding of how upgradeable proxy patterns should work.

Vulnerability Details

In the TokenManager contract (and its parent TokenManagerStorage):

contract TokenManagerStorage is UpgradeableStorage {
/// @dev wrapped native token
address public wrappedNativeToken;
​
/// @dev user token balance can be claimed by user.
/// @dev userTokenBalanceMap[accountAddress][tokenAddress][tokenBalanceType]
mapping(address => mapping(address => mapping(TokenBalanceType => uint256)))
public userTokenBalanceMap;
​
/// @dev token white list
mapping(address => bool) public tokenWhiteListed;
​
// ... other storage variables
}
​
contract TokenManager is TokenManagerStorage, Rescuable, Related, ITokenManager {
// ... logic implementation
}

These storage variables are defined in the logic contract, which will be replaced during an upgrade, resulting in loss of all stored data.

function deployUpgradeableProxy(
uint8 _relatedContractIndex,
address _logic,
bytes memory _data
) external onlyGuardian returns (address) {
// ...
UpgradeableProxy _proxy = new UpgradeableProxy(
@> _logic, // @audit - What if I upgrade to a new logic?
guardian,
address(this),
_data
);
@> relatedContracts[_relatedContractIndex] = address(_proxy); // @audit - Where did the old logic go?
​
// ...
}

In a proper upgradeable proxy pattern:

  1. The proxy contract should hold the state (storage variables).

  2. The logic contract should be stateless and only contain the logic to manipulate the state.

  3. The proxy delegates all calls to the logic contract, which reads and writes state as if it were its own, but the state actually resides in the proxy.

The current implementation not only fails to leverage the upgradeable design but also guarantees complete state loss upon any upgrade, as the new logic contract would start with fresh, uninitialized storage.

Proof of Code

Add the following test in PreMarkets.t.sol. Be sure to bring the necessary imports and modify the variable initializations.

This proof of code illustrates the critical issue of state loss when storage variables are defined in the logic contract instead of the proxy. The example uses the wrappedNativeToken, but the same issue applies to other variables like userTokenBalanceMap.

import { RelatedContractLibraries } from "../src/libraries/RelatedContractLibraries.sol";
​
contract PreMarketsTest is Test {
using RelatedContractLibraries for TadleFactory;
​
TadleFactory tadleFactory;
// ...
tadleFactory = new TadleFactory(user1); // Remove the TadleFactory type in the setUp
​
​
function testStateLossOnUpgrade() public {
bytes memory deploy_data = abi.encodeWithSelector(
INITIALIZE_OWNERSHIP_SELECTOR,
user1
);
// Initial State
address tokenManagerAddressBefore = address(tadleFactory.getTokenManager());
address wrappedTokenBefore = TokenManager(tokenManagerAddressBefore).wrappedNativeToken();
// Deploy a new logic contract (simulating a logic upgrade)
TokenManager newTokenManagerLogic = new TokenManager();
// Upgrade the proxy to the new logic contract
vm.prank(user1);
tadleFactory.deployUpgradeableProxy(
5,
address(newTokenManagerLogic),
bytes(deploy_data)
);
// Check the state after the upgrade
address tokenManagerAddressAfter = address(tadleFactory.getTokenManager());
address wrappedTokenAfter = TokenManager(tokenManagerAddressAfter).wrappedNativeToken();
// Verify that the wrapped token address has changed after the upgrade, indicating state loss
assertTrue(wrappedTokenBefore != wrappedTokenAfter, "Wrapped token address should change after upgrade");
assertEq(wrappedTokenAfter, address(0), "Wrapped token address should be reset to zero after upgrade");
}

Impact

  • Guaranteed State Loss: Every upgrade will result in complete loss of all stored data, as the new logic contract will have its own, uninitialized storage.

  • Broken Functionality: After an upgrade, all functions that rely on previously stored data will either revert or operate on default values, potentially leading to critical failures or unexpected behaviors.

  • Impossible Upgrades: It's effectively impossible to perform a meaningful upgrade without losing all existing data, making the entire upgrade mechanism futile.

  • Potential Fund Lock: If the contract handles any assets or critical permissions based on stored data, these could become permanently inaccessible after an upgrade.

Tools Used

Manual Review

Recommendations

  1. Redesign the upgrade mechanism to follow the standard proxy pattern:

    • Move all storage variables to the proxy contract.

    • Make the logic contract stateless, only containing functions to manipulate state.

    • Ensure the proxy contract uses delegatecall to forward calls to the logic contract.

  2. Implement a proper storage layout in the proxy.

contract UpgradeableTokenManager is UpgradeableProxy {
// Storage layout
address public implementation;
address public admin;
address public tadleFactory;
// TokenManager specific storage
address public wrappedNativeToken;
mapping(address => mapping(address => mapping(uint8 => uint256))) public userTokenBalanceMap;
mapping(address => bool) public tokenWhiteListed;
​
// ... other storage variables
​
// Upgrade function
function upgradeToAndCall(address newImplementation) onlyGuardian external {
implementation = newImplementation;
}
}

3. Modify the logic contract to be stateless.

contract TokenManagerLogic {
function initialize(address _wrappedNativeToken) external {
UpgradeableTokenManager(address(this)).wrappedNativeToken = _wrappedNativeToken;
}
​
// ... other functions that manipulate state through the proxy
}

4. Update the TadleFactory to work with this new design, ensuring it interacts correctly with the proxy and logic contracts.

5. Implement thorough testing of the upgrade process to ensure state preservation and correct functionality after upgrades.

By implementing these changes, the system can achieve true upgradeability, preserving state across upgrades and maintaining system consistency.

Updates

Lead Judging Commences

0xnevi Lead Judge
10 months ago
0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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