SickleFactory Manual Review Findings

SFY-01M: On-Chain Race Condition of Sickle Deployment Approved Address

Type	Severity	Location
Logical Fault		SickleFactory.sol:L157-L159, L167-L169

Description:

The SickleFactory is meant to permit the deployment of Sickle instances that are either deployed by a user themselves or by a contract that integrates with it and has been authorized as a whitelisted caller.

The SickleFactory::getOrDeploy function will evaluate whether a Sickle already exists for a particular user and yield it instead of deploying one as an optimization measure. The Sickle queried is not validated, however, which paves way for an on-chain race condition vulnerability to manifest.

In detail, a user can detect a potential sensitive SickleFactory::getOrDeploy call within a complex transaction (i.e. a migration), and deploy a Sickle ahead of time with a different approved member and different referralCode, the former of which is of the highest severity.

The vulnerability is presently exploitable due to an oversight in the StrategyModule implementation. Specifically, the StrategyModule::getOrDeploySickle function has been defined as public and the contract itself is inherited by multiple strategies such as the FarmStrategy, the LendingMigrator, the LendingStrategy, and more.

As the function is exposed by thee relevant strategy contracts and they are authorized to deploy Sickle implementations, this permits arbitrary Sickle configurations to be deployed and thus a user to successfully submit a transaction that creates a sensitive Sickle before a user interacts with the system. As a potential example exploitation, a malicious user can deploy the Sickle a migration occurs towards with themselves as the approved party to drain funds.

Impact:

An incorrectly exposed function permits a malicious user to deploy a Sickle with a different configuration on behalf of another administrator, resulting in the funds deposited to the Sickle due to the transaction (i.e. a migration) to be compromised by the malicious approved member.

Example:

Copy
151/// @notice Deploys a new Sickle contract for a specific user, or returns
152/// the existing one if it exists
153/// @param admin Address receiving the admin rights of the Sickle contract
154/// @param referralCode Referral code for the user
155/// @return sickle Address of the deployed Sickle contract
156function getOrDeploy(
157    address admin,
158    address approved,
159    bytes32 referralCode
160) external returns (address sickle) {
161    if (!isActive) {
162        revert NotActive();
163    }
164    if (!registry.isWhitelistedCaller(msg.sender)) {
165        revert CallerNotWhitelisted(msg.sender);
166    }
167    if ((sickle = _getSickle(admin)) != address(0)) {
168        return sickle;
169    }
170    return _deploy(admin, approved, referralCode);
171}

Recommendation:

As the StrategyModule::getOrDeploySickle function is solely meant to be utilized internally by upward dependencies and with a correct msg.sender as the first argument, we strongly advise the function to be made internal as a primary remediation.

As a secondary and permanent remediation, we advise the SickleFactory::getOrDeploy function to guarantee that the approved and referralCode values of the SickleFactory::_getSickle result match thus preventing the usage of a potentially malicious Sickle implementation.

Alleviation (6ab7af3bb495b817ffec469255ea679b1813eecb):

The vfat team evaluated this exhibit and has opted to alleviate it in a future version as they consider the impact limited to users who interact directly with the vfat system.

Based on the fact that the vfat team has adequate security measures in their front-end during the deployment of Sickles to avoid this race-condition and that they intend to address it in the future, we consider the exhibit to be safely acknowledged.