Centralization risk - administrators can steal all basetokens in collateral by modifying the DepositRecord address

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83

Vulnerability details

Impact

Centralization risk - administrators can steal all basetokens in collateral by modifying the DepositRecord address

Proof of Concept

In the collateral contract, allow the manager to transfer basetoken by calling managerWithdraw()

  function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);
    baseToken.transfer(manager, _amount);
  }

But there is a limitation in managerWithdrawHook.hook

  function hook(address, uint256, uint256 _amountAfterFee) external view override { require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum"); }
  function getMinReserve() public view override returns (uint256) { return (depositRecord.getGlobalNetDepositAmount() * minReservePercentage) / PERCENT_DENOMINATOR; }

After transferring basetoken, the remaining number of basetoken must be greater than the value of getMinReserve(). But here the administrator can temporarily change DepositRecord to a new address by bribing SET_DEPOSIT_RECORD_ROLE. Then the return value of getMinReserve() is 0. Thus administrators can bypass restrictions in hook(). Steal all basetoken in the contract.

Tools Used

vscode

Recommended Mitigation Steps

Add time lock or prohibit modification of depositrecord address

[c4-judge]

Picodes marked the issue as duplicate of #254

[c4-judge]

Picodes marked the issue as satisfactory