r4bbit - Proxy contracts storage layout can be corrupted on upgradable contracts

[sherlock-admin2]

r4bbit

medium

Proxy contracts storage layout can be corrupted on upgradable contracts

Summary

The Sophon Farming Contracts make use of a proxy system that is also known as unstructured storage proxy pattern. When using this pattern, it's important to keep in mind that there are possible storage collisions when the implementation contract is upgraded and changes are made to the storage layout. The contracts used in this protocol are at risk to run into storage collisions in the proxy contract.

Vulnerability Detail

The unstructured proxy storage pattern is generally implemented by having at least two contracts: A contract that implements the application logic, known as the "logic" or "implementation" contract, and a proxy contract, which is the contract that accounts will interact with directly.

The proxy contracts keep a reference to the implementation contract, and forwards incoming calls to the implementation contract. This allows for upgradability, as protocols can deploy a new implementation contract and have the proxy contract point to it, resulting in calls to the proxy contract being forwarded to the new implementation contract.

What's crucial, is that forwarding is done using DELEGATECALL, which means the execution context will use the storage of the proxy contract, not the implementation contract.

We can find equivalent implementations of these contracts in the following files:

• Proxy2Step - A proxy contract with Upgradeable2Step capabilities
• SophonFarmingProxy - Inherits Proxy2Step
• SophonFarming - The implementation contract, also with Upgradeable2Step capabilities

There's also a Proxy to which everything that's described here applies there as well. Given that SophonFarmingProxy uses Proxy2Step, we'll focus on that proxy implementation in this submission. However, consider the mitigation steps described below for Proxy as well.

When a proxy for SophonFarming is created, it receives a reference to the implementation contract in its constructor.

contract Proxy2Step is Upgradeable2Step {

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

This results in the implementation being stored in storage slot 3 of the contract. Storage slots 0-2 are occupied by storage variables introduced by Upgradeable2Step and Ownable2Step (which Upgradeable2Step inherits from). To verify this, we can take a look at the contract's storage layout:

❯ forge inspect SophonFarmingProxy storage --pretty
| Name                  | Type    | Slot | Offset | Bytes | Contract                                                    |
|-----------------------|---------|------|--------|-------|-------------------------------------------------------------|
| _owner                | address | 0    | 0      | 20    | contracts/proxies/SophonFarmingProxy.sol:SophonFarmingProxy |
| _pendingOwner         | address | 1    | 0      | 20    | contracts/proxies/SophonFarmingProxy.sol:SophonFarmingProxy |
| pendingImplementation | address | 2    | 0      | 20    | contracts/proxies/SophonFarmingProxy.sol:SophonFarmingProxy |
| implementation        | address | 3    | 0      | 20    | contracts/proxies/SophonFarmingProxy.sol:SophonFarmingProxy |

Currently SophonFarming also inherits Upgradeable2Step, which means its storage slots 0-3 are the same:

❯ forge inspect SophonFarming storage --pretty
| Name                   | Type                                | Slot | Offset | Bytes | Contract                                       |
|------------------------|-------------------------------------|------|--------|-------|------------------------------------------------|
| _owner                 | address                             | 0    | 0      | 20    | contracts/farm/SophonFarming.sol:SophonFarming |
| _pendingOwner          | address                             | 1    | 0      | 20    | contracts/farm/SophonFarming.sol:SophonFarming |
| pendingImplementation  | address                             | 2    | 0      | 20    | contracts/farm/SophonFarming.sol:SophonFarming |
| implementation         | address                             | 3    | 0      | 20    |

However, there can easily be a storage collision between the state variables defined in the proxy contract and the implementation contract, when the implementation contract is upgraded and it doesn't adhere to the current storage layout.

This includes:

• When changing the variables order in the contract
• When removing the non-latest variable defined in the contract
• When changing the type of a variable
• When introducing a new variable before any existing one

For storage variables related to proxy functionality, it's better to rely on consistent pseudo-random storage slots, such as proposed by ERC-1967.

Impact

Depending on the actions taken changes made by developers when deploying and upgrading the implementation contract, this could cause loss of user funds and failure of the protocol, as corrupting the storage can result in malformed data.

Code Snippet

Below is a proof of concept that shows how the proxy's storage can be accidentally corrupted.

Imagine a new upgrade being performed for SophonFarming contract. This new version SophonFarmingStorageCollision contract does not inherit Upgradeable2Step for unknown reasons, but also makes use of storage variables that happen to use the same slot as the proxy's implementation:

contract SophonFarmingStorageCollision is Ownable2Step {

    // same slot as SophonFarmingProxy.pendingImplementation
    address public pendingImplementationStorage;

    // same slot as SophonFarmingProxy.implementation
    address public implementationStorage;

    constructor() Ownable(msg.sender) {}

    function badLogic() public {
        // Some logic that accidentally writes to the same storage slot as the proxy
        pendingImplementationStorage = address(0);
        implementationStorage = address(0);
    }

    function becomeImplementation(Upgradeable2Step proxy) public {
        require(proxy.owner() == msg.sender, "Unauthorized"");
        proxy.acceptImplementation();
    }
}

To confirm, here's what the storage layout of this contract looks like:

❯ forge inspect SophonFarmingStorageCollision storage --pretty
| Name                         | Type    | Slot | Offset | Bytes | Contract                                                                       |
|------------------------------|---------|------|--------|-------|--------------------------------------------------------------------------------|
| _owner                       | address | 0    | 0      | 20    | contracts/farm/SophonFarmingStorageCollision.sol:SophonFarmingStorageCollision |
| _pendingOwner                | address | 1    | 0      | 20    | contracts/farm/SophonFarmingStorageCollision.sol:SophonFarmingStorageCollision |
| pendingImplementationStorage | address | 2    | 0      | 20    | contracts/farm/SophonFarmingStorageCollision.sol:SophonFarmingStorageCollision |
| implementationStorage        | address | 3    | 0      | 20    | contracts/farm/SophonFarmingStorageCollision.sol:SophonFarmingStorageCollision |

Then, the following scenario:

function test_ProxyStorageCollisionVulnerability() public {
    // impersonate deployer
    vm.startPrank(deployer);

    // deployer new version of sophon farming implementation contract
    address newImplementation = address(
        new SophonFarmingStorageCollision()
    );

    // signal to proxy that implementation should be changed
    sophonFarmingProxy.replaceImplementation(newImplementation);
    assertEq(sophonFarmingProxy.pendingImplementation(), newImplementation);

    // accept new implementation of farming contract
    SophonFarmingStorageCollision(payable(newImplementation)).becomeImplementation(sophonFarmingProxy);

    assertEq(sophonFarmingProxy.implementation(), newImplementation);

    // make call to implementation contract's `badLogic()` which happens to override storage slot of proxy
    (bool success, ) = address(sophonFarmingProxy).call(abi.encodeWithSignature("badLogic()"));
    assert(success);

    // verify that proxy's implementation reference is now bricked
    assertEq(sophonFarmingProxy.implementation(), address(0));
}

Output:

Ran 1 test for test/SophonFarming.t.sol:SophonFarmingTest
[PASS] test_ProxyStorageCollisionVulnerability() (gas: 284620)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.96ms (88.71µs CPU time)

Ran 1 test suite in 142.36ms (3.96ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tool used

• Manual Review
• Foundry for testing

Recommendation

Consider making use of OpenZeppelin's ERC1967Proxy contract. It properly implement ERC1967 to reduce the likelyhood of storage collisions in proxies and also implements additional safety considerations.

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

+     constructor(address impl_) ERC1967Proxy(impl_, "") {}
-     constructor(address impl_) {
-         implementation = impl_;
-     }

...
-    fallback() external virtual payable {
-        assembly {
-            // @audit this should copy into free memory pointer location instead
-            calldatacopy(0, 0, calldatasize())
-            // @audit read from free memory pointer location instead
-            let result := delegatecall(gas(), sload(implementation.slot), 0, calldatasize(), 0, 0)
-            // @audit this should copy into free memory pointer location instead
-            returndatacopy(0, 0, returndatasize())
-            switch result
-            case 0 { revert(0, returndatasize()) }
-            default { return(0, returndatasize()) }
-        }
-    }
...
}

[sherlock-admin2]

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

0xmystery commented:

invalid because while the current design is prone to storage collision, the likelihood is slim