search

sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

Tricky Pebble Dachshund - Upgradeable contracts should derive from openzeppelin upgradeable library only as best practice #697

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

Tricky Pebble Dachshund

Low/Info

Upgradeable contracts should derive from openzeppelin upgradeable library only as best practice

Summary

To avoid any storage layout related conflict, the Upgradeable should derive from openzeppelin upgradeable library contract only, not the other standard library. MlumStaking contract derives from ReentrancyGuard which does not follow the best practices.

Vulnerability Detail

MlumStaking contract derives from ReentrancyGuard which is not part of openzeppelin upgradeable library contracts.

Impact

No Impact in this context.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L31-L40

  contract MlumStaking is
    Ownable2StepUpgradeable,
 ==>ReentrancyGuard,  // @audit - should use upgradable version
    IMlumStaking,
    ERC721Upgradeable,
    ERC721EnumerableUpgradeable
{
    using EnumerableSet for EnumerableSet.AddressSet;
    using SafeERC20 for IERC20;

Tool used

Manual Review

Recommendation

The recommendation is to use the version from upgradeable library.

openzeppelin-contracts-upgradeable

0xHans1 commented 3 months ago

https://github.com/metropolis-exchange/magicsea-staking/pull/16