dimi6oni - Lack of additional verification for `Upgradeable2Step::acceptImplementation` leads to unauthorized implementation acceptance

[sherlock-admin4]

dimi6oni

medium

Lack of additional verification for Upgradeable2Step::acceptImplementation leads to unauthorized implementation acceptance

Summary

The acceptImplementation function in the Upgradeable2Step contract lacks additional verification steps to ensure the legitimacy of the pendingImplementation, potentially allowing unauthorized implementation changes if the pendingImplementation address is compromised or malicious.

Vulnerability Detail

The acceptImplementation function in the Upgradeable2Step contract can be called by any address that matches pendingImplementation.

This function should be restricted to the owner or another trusted entity to prevent unauthorized changes.

Impact

An attacker who gains control of the pendingImplementation address could replace the implementation contract with a malicious one, potentially stealing funds or disrupting the protocol's functionality.

Code Snippet

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

Tool used

Manual Review

Recommendation

Implement additional verification mechanisms to ensure the legitimacy of the pendingImplementation. This could include multisignature approval, off-chain verification, or other methods to verify the identity and integrity of the pendingImplementation.

For example, you could add a multi-step verification process:

• Require multiple confirmations from trusted parties before allowing the acceptImplementation call.
• Introduce a delay between setting the pendingImplementation and allowing the acceptImplementation call, giving time to detect and react to any compromise.

// An example approach using a two-step verification with a delay

error ChangeDelayNotElapsed();

contract Upgradeable2Step is Ownable2Step {

+ uint256 public implementationChangeDelay = 1 days;
+ uint256 public changeInitiatedAt;
    address public pendingImplementation;
    address public implementation; 

    constructor() Ownable(msg.sender) {}

    function replaceImplementation(address impl_) public onlyOwner {
        pendingImplementation = impl_;

+ changeInitiatedAt = block.timestamp;
        emit ReplaceImplementationStarted(implementation, impl_);
    }

function acceptImplementation() public {
        if (msg.sender != pendingImplementation) {
            revert OwnableUnauthorizedAccount(msg.sender);
        }

+ if (block.timestamp < changeInitiatedAt + implementationChangeDelay) {
revert ChangeDelayNotElapsed();
}

        emit ReplaceImplementation(implementation, msg.sender);
        delete pendingImplementation;
        implementation = msg.sender;
    }

[sherlock-admin2]

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

0xmystery commented:

invalid because admins are trusted and will mitigate the risk of a malicious implementation contract if need be