sherlock-audit / 2024-05-sophon-judging

1 stars 1 forks source link

dimi6oni - Uninitialized proxy attack causes unauthorized access and control over the contract #194

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

dimi6oni

medium

Uninitialized proxy attack causes unauthorized access and control over the contract

Summary

The proxy contract, Proxy2Step, lacks an initialization mechanism to set crucial initial state variables. This oversight can potentially allow an attacker to exploit the uninitialized state of the proxy contract, resulting in unauthorized access and control over the contract.

Vulnerability Detail

The Proxy2Step contract does not implement an initialize function to set up important state variables, such as implementation, in a secure manner. In the current implementation, the constructor of Proxy2Step directly sets the implementation address, but this does not prevent an attacker from exploiting the uninitialized state before the constructor has been called, particularly if the contract is deployed without proper initialization sequences. There's a brief moment where the contract could potentially be called before the constructor completes its execution. Attackers might exploit this small window to execute functions before the implementation address is set.

Furthermore, during this initialization phase, the constructor of the contract is executed. Normally, during this phase, no external interaction should occur. However, certain vulnerabilities and edge cases can arise, especially with complex deployment mechanisms or when proxy patterns are involved.

• In some complex scenarios involving reentrancy or delegatecall mechanisms, especially with proxy contracts, initialization functions might inadvertently be called before the constructor completes. • If the deployment involves multiple steps or if the contracts are deployed in an order that allows temporary inconsistencies, an attacker could exploit the brief window when the contract is not fully initialized. • In past incidents, improper initialization sequences led to vulnerabilities. These incidents often involved proxy patterns where initialization functions were not properly restricted or not present at all. Such are the cases with Wormhole and Harvest Finance Uninitialized Proxy.

Impact

If the proxy contract is not properly initialized, it could lead to severe security issues, like unauthorized users may be able to call critical functions and gain control over the contract; attackers could replace the implementation with a malicious contract; or the entire protocol's funds could be at risk due to unauthorized actions on the proxy contract.

Code Snippet

The vulnerability lies in the Proxy2Step constructor and the absence of an initialize function.

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/proxies/Proxy2Step.sol#L8-L9

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, even with onlyOwner checks in Upgradeable2Step, it is good practice to use an explicit initialize function protected by an initializer modifier to set up the contract state securely. The contract should use OpenZeppelin's Initializable pattern to prevent re-initialization.

+   import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
    import "./Upgradeable2Step.sol";

+ error InvalidImplementationAddress();

+ contract Proxy2Step is Initializable, Upgradeable2Step {
- contract Proxy2Step is Upgradeable2Step {

-   constructor(address impl_) {
-       implementation = impl_;
-   }

// The initialize function to set the initial state securely
+    function initialize(address impl_) public initializer {
+ if (impl_ == address(0)) {
+       revert InvalidImplementationAddress();
+   }
+ implementation = impl_;
+ }
sherlock-admin4 commented 1 month ago

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

invalid because Front-running initializers where there is no irreversible damage or loss of funds & the protocol could just redeploy and initialize again is not a valid issue.