tokamak-network / Tms-contract

GNU General Public License v3.0
0 stars 1 forks source link

Be sure to add a test script that upgrades the implementation of the proxy contract. #4

Closed usgeeus closed 2 weeks ago

usgeeus commented 2 months ago

Describe the bug I heard that you chose to deploy this contract in a proxy pattern. If this is a proxy pattern, and if we want to use it at production level, we need to make sure that the implementation of the proxy is upgradeable.

Configuration

*Impact If an incident occurs, and if it cannot be upgraded or paused, there is nothing we can do

Recommendation Add a test script that upgrades the implementation of the proxy contract. upgradeTo, upgradeToAndCall, changeAdmin needs to be tested.

shivtokamak commented 4 weeks ago

Fixed , pls check

usgeeus commented 4 weeks ago

@shivtokamak I checked your test code, thanks. But why use the proxy pattern if nothing is stored as contract states???

shivtokamak commented 3 weeks ago

It was to add new functions for future like senderc721 ,etc

usgeeus commented 3 weeks ago

Then you can just deploy a new contract, I'm asking because I'm not sure there's enough motivation to use a proxy pattern. Proxy pattern delegateCalls functions using more gas.

This is not a contract security issue, so if you decided to use the proxy pattern anyway, then you can close this issue.

Zena-park commented 2 weeks ago

In the emergency cases such as when funds are locked(If someone accidentally sends funds or there is a logic error.), it is safe to use a proxy pattern.

shivtokamak commented 2 weeks ago

Please check, added rescue erc20 function

usgeeus commented 2 weeks ago

Thanks