Possible Denial-Of-Service of minting functionality in `CvxStakingPositionManager`

[hats-bug-reporter[bot]]

Github username: @0xpessimist Twitter username: 0xpessimist Submission hash (on-chain): 0x7f0f5703e47bcc4533094462af3979eb240ecc8f5f119f32a13a8eeac5e15fc9 Severity: medium

Description: Description\ In the initialize() function of CvxStakingPositionManager, nextId = 1; is set, but the mint() function can also operate when the contract is uninitialized, so if we mint until nextId = 2 before CvxStakingPositionManager is initialized (perhaps by frontrunning), when initialize() sets nextId to 1 the mint() function will revert and no more mints can be executed on this contract.

Attack Scenario\

• Alice notices that the CvxStakingPositionManager contract has not been initialized (whether it's in the mempool or not, the attack can be performed in both cases).

• Alice executes the mint() function within the same transaction until nextId = 2.

• No further mint operations can be performed on CvxStakingPositionManager because the mint must satisfy the following two conditions:

  • tokenId must not exist.
  • to cannot be the zero address.

Attachments

1. Proof of Concept (PoC) File

CvxStakingPositionManager.sol#L59

Convex/CvxStakingPositionManager.sol#L140

2. Mitigation

A modifier or a require line should be added to prevent the mint() function from being executed before CvxStakingPositionManager is initialized. This can be done with a boolean variable that checks whether it has been initialized or not. Additionally, other functions that could cause issues similar to mint should be looked into and if necessary, the same precautions should be taken for them.

[PlamenTSV]

The function has proper access control and cannot be called before initialization. https://github.com/hats-finance/Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb/blob/b3c66b1323cd555f5fa784b12736fd9b8f9ddfc5/contracts/Staking/Convex/CvxStakingPositionManager.sol#L138

[0xpessimist]

The initialize() function in CvxStakingPositionManager has nothing to do with CvgControlTower.isStakingContract() check, also there is no setting for this in initialize already. I would accept this if there was a situation like "isStakingContract cannot be true before CvxStakingPositionManager is initialized", but isStakingContract being true depends on the insertNewSdtStaking() function and this is already set to true when creating in CloneFactory;

https://github.com/hats-finance/Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb/blob/b3c66b1323cd555f5fa784b12736fd9b8f9ddfc5/contracts/CloneFactory.sol#L104

https://github.com/hats-finance/Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb/blob/b3c66b1323cd555f5fa784b12736fd9b8f9ddfc5/contracts/CvgControlTower.sol#L152-L157

So I think that require will not protect against what I mentioned.

[PlamenTSV]

You can read the deploy scripts if you don't believe me, no protocol would initialize their staking contracts before first initializing their dependency contracts, in our case being the NFT contract. This is an admin error/centralization risk at best since there is no other way it could occur, which by the guidelines of the contest would make it OOS.