Initialize function missing modifiers

[code423n4]

Lines of code

https://github.com/maple-labs/loan/blob/v3.0.0-beta.1/contracts/MapleLoanInitializer.sol#L48 https://github.com/maple-labs/loan/blob/v3.0.0-beta.1/contracts/MapleLoanInternals.sol#L102-L136

Vulnerability details

Impact

MapleLoanInternals.sol has an internal _initialize function. This function is called by MapleLoanInitializer.sol in the fallback function, which is an external function. There is no modifier or access control on the fallback function. Anyone can call the fallback function, which calls _initialize, to set the state variables in MapleLoanInternals.sol, including _paymentInterval, _collateralRequired, _interestRate, and others.

Proof of Concept

The internal _initialize function is called in this fallback function. The fallback function does not limit who is able to call it https://github.com/maple-labs/loan/blob/v3.0.0-beta.1/contracts/MapleLoanInitializer.sol#L48

The _initialize function sets a dozen or more state variables https://github.com/maple-labs/loan/blob/v3.0.0-beta.1/contracts/MapleLoanInternals.sol#L102-L136

Tools Used

Manual analysis

Recommended Mitigation Steps

If the fallback function serves a purpose after initialization, put the _initialization function call in a different location than in the fallback function. Use the initializer modifier from the Open Zeppelin Initializable.sol contract on the internal _initializer function https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol

[JGcarv]

This is not really a bug because this function can only be called during a createInstance transaction in the maple factory.

[lucas-manuel]

https://github.com/maple-labs/loan/wiki/Proxies-and-Upgradeability#mapleloan-proxy-deployment

The fallback function in the MapleLoanInitializer can be called to update the statue of the MapleLoanInitializer itself, but that is harmless since they are just state variables and the fallback is the only external function in the contract. There is no way to self-desctruct etc this contract by manipulating the storage.

In order to manipulate the storage in the context of an actually deployed loan, the only way to do that is by calling migrate which is permissioned to the MapleLoanFactory.

This is not a valid issue.

[dmvt]

This could leave open hypothetical attacks you're not thinking of. Why not just permission it or remove the call to initialize? Regardless, this is out of scope so invalid.