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

The `SystemConfig.sol` does not have protection against re-initialization

spomaria

Summary

Upgradeable contracts use the initialize function to initialize state variables instead of the constructor in traditional contracts. Similarly, the SystemConfig.sol contract uses the initialize function to initialize the SystemConfigStorage::basePlatformFeeRate and SystemConfigStorage::baseReferralRate variables.

Vulnerability Details

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

function initialize(
uint256 _basePlatformFeeRate,
uint256 _baseReferralRate
) external onlyOwner {
basePlatformFeeRate = _basePlatformFeeRate;
baseReferralRate = _baseReferralRate;
}

Thus, the contract can be re-intialized changing the initial values of the state variables.

Impact

When SystemConfig.sol is re-initialized using the SystemConfig::initialize function, it results in different SystemConfigStorage::basePlatformFeeRate and SystemConfigStorage::baseReferralRate values 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_SystemConfig_Can_ReInitialize() public {
uint256 baseFeeBefore = systemConfig.basePlatformFeeRate();
uint256 referralFeeBefore = systemConfig.baseReferralRate();
​
// re-initialize the TokenManager contract
uint256 basePlatformFeeRate2 = 5_00;
uint256 baseReferralRate2 = 30_000;
​
vm.prank(user1);
systemConfig.initialize(basePlatformFeeRate2, baseReferralRate2);
​
uint256 baseFeeAfter = systemConfig.basePlatformFeeRate();
uint256 referralFeeAfter = systemConfig.baseReferralRate();
​
assertEq(baseFeeBefore, baseFeeAfter);
assertEq(referralFeeBefore, referralFeeAfter);
}

Run: forge test --match-test test_SystemConfig_Can_ReInitialize

Output:

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

Thus the values of the state variables have changed after re-initialization.

Recommendations

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

+ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
.
.
.
- contract SystemConfig is SystemConfigStorage, Rescuable, ISystemConfig {
+ contract SystemConfig is SystemConfigStorage, Rescuable, ISystemConfig, Initializable {
.
.
.
function initialize(
uint256 _basePlatformFeeRate,
uint256 _baseReferralRate
- ) external onlyOwner {
+ ) external onlyOwner initializer {
basePlatformFeeRate = _basePlatformFeeRate;
baseReferralRate = _baseReferralRate;
}

This way, the SystemConfig.sol contract can only be initiallized once.

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.