search

sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

Shawon - Upgradable implementation Contracts should not have `constructor` in them #687

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

Shawon

Invalid

Upgradable implementation Contracts should not have constructor in them

name: Upgradable implementation Contracts should not have constructor in them

title: "Upgradable implementation Contracts should not have constructor in them" labels: "High"

Summary : Upgradable smart contracts have 2 parts. One of them is the implementation and the other one is the Proxy contract.The implementation contract holds all the logics and gets replaced every time the cotracts gets updated. That's why we use initialzers to set up the implementation contract and all the storage stays in the proxy contract which points to the implementation contract.

Vulnerability Detail : By using a constructor in an implementation contract, we are risking losing valuable storage data when we upgrade our smart contract. Also, proxies are not well equipped to access storage from the implementation contracts. So, the risks remain no matter how small amount of data we store using the constructor.

Impact: Using constructors, we store information in that particular contract and thus when we replace that contract with a new one we risk of losing it forever even if we again deploy another contract, the proxies are not that capable to access them properly and there is still a risk of storage collison.

Code Snippet: BaseRewarder.sol has this

`constructor(address caller) { _caller = caller; implementation = address(this); //@audit as this is an implementation contract, this shouldn't have any constructor

    _disableInitializers();
}

/**
 * @dev Initializes the BaseRewarder contract.
 * @param initialOwner The initial owner of the contract.
 */
function initialize(address initialOwner) public virtual override initializer {
    //@audit the initialize function can be called more than one time.
    __Ownable_init(initialOwner);
}`

BribeRewarder.sol has this

`constructor(address caller) { _caller = caller; implementation = address(this);

    _disableInitializers();
}

/**
 * @dev Initializes the BaseRewarder contract.
 * @param initialOwner The initial owner of the contract.
 */
function initialize(address initialOwner) public virtual initializer {
    __Ownable_init(initialOwner);
}

`

Tool used

Manual Review

Recommendation: We highly encourage to store what ever information inside logic contracts using initializer function. As it does exacly the same with a bit more modualrity and it follows the basic standars of upgradable smart contracts.

0xSmartContract commented 3 months ago

invalid