Improper storage slot management in proxy contract causes potential overwriting of critical variables

Summary

The proxy and implementation contracts may face issues due to the direct storage of the implementation address in a common storage slot. This approach lacks the safeguards recommended by EIP-1967, potentially leading to storage collisions with the implementation contract.

Vulnerability Detail

The implementation address is set in the constructor of the Proxy2Step contract and stored using the implementation variable from the Upgradeable2Step contract. This method of storing the implementation address does not protect against storage collisions, which can occur if the implementation contract inadvertently uses the same storage slots as the proxy, particularly during upgrades or when extending functionality.

Impact

Storage collisions between the proxy and its implementation can lead to unpredictable behavior, including overwriting critical variables. This could compromise the integrity of the contract's logic, lead to loss of funds, or cause disruption in the protocol's operation.

Code Snippet

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

Tool used

Manual Review

Recommendation

To mitigate the risk of storage collisions in a transparent proxy setup:

Adopt EIP-1967 Standard: Modify the storage of critical administrative variables like implementation to use specific, pre-determined storage slots based on hashed identifiers. This helps ensure that these slots will not collide with those used by the implementation contracts.
Manage Storage Layout Carefully: Ensure that during contract upgrades, both the proxy and implementation maintain a consistent and non-overlapping storage layout.

Upgradeable2Step Contract:

pragma solidity 0.8.25;

import "@openzeppelin/contracts/access/Ownable2Step.sol";

event ReplaceImplementationStarted(address indexed previousImplementation, address indexed newImplementation);
event ReplaceImplementation(address indexed previousImplementation, address indexed newImplementation);
error Unauthorized();

contract Upgradeable2Step is Ownable2Step {
    // This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1
+   bytes32 private constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
    address public pendingImplementation;
-   address public implementation;

    constructor() Ownable(msg.sender) {}

+   function implementation() public view returns (address impl) {
+       bytes32 slot = _IMPLEMENTATION_SLOT;
+       assembly {
+           impl := sload(slot)
+       }
+   }

    // Update the implementation address in the designated EIP-1967 slot
    function replaceImplementation(address impl_) public onlyOwner {
       pendingImplementation = impl_;
+       emit ReplaceImplementationStarted(implementation(), impl_);
-               emit ReplaceImplementationStarted(implementation, impl_);
    }

    // Accept the new implementation if called from the pending implementation
    function acceptImplementation() public {
        if (msg.sender != pendingImplementation) {
           revert OwnableUnauthorizedAccount(msg.sender);
        }
+       bytes32 slot = _IMPLEMENTATION_SLOT;
+       assembly {
+           sstore(slot, pendingImplementation)
+       }
                emit ReplaceImplementation(implementation(), msg.sender);
        delete pendingImplementation;
-       implementation = msg.sender;
    }

    // The existing becomeImplementation function is fine as it checks the owner and then calls acceptImplementation
}

Proxy2Step Contract:

pragma solidity 0.8.25;

import "./Upgradeable2Step.sol";

contract Proxy2Step is Upgradeable2Step {

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

    // Internal function to set the implementation address
+   function _setImplementation(address newImplementation) internal {
+       bytes32 slot = _IMPLEMENTATION_SLOT;
+       assembly {
+           sstore(slot, newImplementation)
+       }
+   }

    // Proxy fallback function using the implementation address from the EIP-1967 slot
    fallback() external virtual payable {
        assembly {
+           let _impl := sload(_IMPLEMENTATION_SLOT)
            calldatacopy(0, 0, calldatasize())
+           let result := delegatecall(gas(), _impl, 0, calldatasize(), 0, 0)
-                     let result := delegatecall(gas(), sload(implementation.slot), 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 { revert(0, returndatasize()) }
            default { return(0, returndatasize()) }
        }
    }

    receive() external virtual payable {}
}

sherlock-audit / 2024-05-sophon-judging

dimi6oni - Improper storage slot management in proxy contract causes potential overwriting of critical variables #200