code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/main/contracts/GlobalImpliedCollateralService.sol#L116

Vulnerability details

Impact

GlobalImpliedCollateralService.setPoolUpdater() removes the updater when _updater == oldUpdater.

Proof of Concept

GlobalImpliedCollateralService.setPoolUpdater() is used to set or update the pool updaters.

  function setPoolUpdater(address _pool, address _updater)
    external
    onlyRoleMalt(UPDATER_MANAGER_ROLE, "Must have updater manager role")
  {
    require(_updater != address(0), "GlobImpCol: No addr(0)");
    poolUpdaters[_updater] = _pool;
    address oldUpdater = poolUpdatersLookup[_pool];
    emit SetPoolUpdater(_pool, _updater);
    poolUpdaters[oldUpdater] = address(0); //@audit doesn't work when _update = oldUpdater
    poolUpdatersLookup[_pool] = _updater;
  }

But it removes the oldUpdater after setting the new updater so it will reset poolUpdaters[_updater] when _updater == oldUpdater.

As a result, the original updater won't have a relevant role.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend checking _update != oldUpdater.

  function setPoolUpdater(address _pool, address _updater)
    external
    onlyRoleMalt(UPDATER_MANAGER_ROLE, "Must have updater manager role")
  {
    require(_updater != address(0), "GlobImpCol: No addr(0)");
    poolUpdaters[_updater] = _pool;
    address oldUpdater = poolUpdatersLookup[_pool];

    require(_updater != oldUpdater, "Same updater"); //+++++++++++++++++++

    emit SetPoolUpdater(_pool, _updater);
    poolUpdaters[oldUpdater] = address(0);
    poolUpdatersLookup[_pool] = _updater;
  }

c4-sponsor commented 1 year ago

0xScotch marked the issue as sponsor confirmed

Picodes commented 1 year ago

Downgrading to Low as this boils down to adding a safety check on an admin function.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

code-423n4 / 2023-02-malt-findings