IRREVERSIBLE __RENOUNCEGUARDIAN FUNCTION

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L345-L350

Vulnerability details

Impact

In AstariaRouter.sol, __renounceGuardian() can be self-called by the existing guardian. With a zero address assigned to guardian, no one will be able to call fileGuardian() forever. This is because a new guardian will have to be set by a valid and existing guardian who is now a zero address.

Proof of Concept

After calling __renounceGuardian(), both the guardian and the new guardian will be assigned zero address:

AstariaRouter.sol#L345-L350

  function __renounceGuardian() external {
    RouterStorage storage s = _loadRouterSlot();
    require(msg.sender == s.guardian);
    s.guardian = address(0);
    s.newGuardian = address(0);
  }

Consequently, fileGuardian() becomes non-callable because it requires msg.sender == address(s.guardian):

AstariaRouter.sol#L359-L391

  function fileGuardian(File[] calldata file) external {
    RouterStorage storage s = _loadRouterSlot();
    require(msg.sender == address(s.guardian));

    uint256 i;
    for (; i < file.length; ) {
      FileType what = file[i].what;
      bytes memory data = file[i].data;
      if (what == FileType.Implementation) {
        (uint8 implType, address addr) = abi.decode(data, (uint8, address));
        if (addr == address(0)) revert InvalidFileData();
        s.implementations[implType] = addr;
      } else if (what == FileType.CollateralToken) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.COLLATERAL_TOKEN = ICollateralToken(addr);
      } else if (what == FileType.LienToken) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.LIEN_TOKEN = ILienToken(addr);
      } else if (what == FileType.TransferProxy) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.TRANSFER_PROXY = ITransferProxy(addr);
      } else {
        revert UnsupportedFile();
      }
      emit FileUpdated(what, data);
      unchecked {
        ++i;
      }
    }
  }

None of the associated what statuses above can be implemented/executed on a permanent basis because setNewGuardian() requires msg.sender == s.guardian:

AstariaRouter.sol#L339-L343

  function setNewGuardian(address _guardian) external {
    RouterStorage storage s = _loadRouterSlot();
    require(msg.sender == s.guardian);
    s.newGuardian = _guardian;
  }

Recommended Mitigation Steps

It is recommended refactoring setNewGuardian() by including another authorized address in its require():

  function setNewGuardian(address _guardian) external {
    RouterStorage storage s = _loadRouterSlot();
    require(msg.sender == s.guardian || msg.sender == owner);
    s.newGuardian = _guardian;
  }

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c