code-423n4 / 2024-05-olas-validation

0 stars 0 forks source link

StakingBase: initialize function is frontrunnable #274

Closed c4-bot-5 closed 4 months ago

c4-bot-5 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingToken.sol#L54-L69 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L278-L361

Vulnerability details

Vulnerability Detail

The Olas flow of Staking contract creation is as follows:

  1. Staking blanc implementation address is created (so it could be passed into the factory). Additionally, the initPayload is encoded so, the Staking blanc implementation could be initialised during the creation in the factory.
  2. In the StakingFactory.sol both this params are passed into the createStakingInstance, which creates the instance via create2

    function createStakingInstance(
            address implementation,
            bytes memory initPayload
        ) external returns (address payable instance)

However, the problem is that implementation contract left uninitialised. It is initialised only after it manually passed into the createStakingInstance function where it is initialised via the newly created proxy

(bool success, bytes memory returnData) = instance.call(initPayload)

The problem is that, during the time-frame when the implementation is left uninitialised, it could be taken by an attacker. And the createStakingInstance function would fail.

Proof of Concept

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingToken.sol#L54-L69

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L278-L361

Impact

The createStakingInstance fail. implementation contract is taken by an attacker

Recommendation

It is recommended to make a necessary access control checks in the implementation contract, so it checks that only stakingFactory address could call the function.

Assessed type

MEV