Submitted by ciphermarco
As per the audit repositorys documentation, which is confirmed as up-to-date, there are carefully considered emergency scenarios. Among these scenarios, one is described as follows:
https://github.com/code-423n4/2023-09-centrifuge/blob/main/README.md?plain=1#L74-L77:
That makes perfect sense from a security perspective. However the provided DelayedAdmin implementation lacks the necessary functionality to execute PauseAdmin.removePauser in the case of an emergency.
Striving to adhere to the documented Severity Categorization, I have categorized this as Medium instead of Low. The reason is that it does not qualify as Low due to representing both a function incorrect as to spec issue and a critical feature missing from the projects security model. Without this emergency action for PauseAdmin, other recovery paths may have to wait for Roots delay period or, at least temporarily, change the protocols security model to make a recovery. In my view, this aligns with the Assets not at direct risk, but the function of the protocol or its availability could be impacted requirement for Medium severity. With that said, I realize the sponsors and judges will ultimately evaluate and categorise it based on their final risk analysis, not mine. Im simply streamlining the process by presenting my perspective in advance.
In order to remove a pauser from the PauseAdmin contract, the removePause function must be called:
https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/admins/PauseAdmin.sol#L39-L42:
Since it is a short contract, here is the whole DelayedAdmin implementation:
https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/admins/DelayedAdmin.sol
No implemented functionalities exist to trigger PauseAdmin.removePauser. Additionally, the contract features an unused event File,
a and fails to record the PauseAdmins address upon initiation or elsewhere.
As anticipated, Auth (inherited) also does not handle this responsibility:
To confirm that I did not misunderstand anything, I thoroughly searched the entire audit repository for occurrences of removePauser.
However, I could only find them in PauseAdmin.sol, where the function to remove a pauser is implemented, and in a test case that
directly calls the PauseAdmin.
1. Implement the PauseAdmin.removePauser Functionality in DelayedAdmin.sol with This Diff:
2. Add This Test Function to AdminTest Contract in test/Admin.t.sol
3. Change the DelayedAdmin Creation in script/Deployer.sol with This diff:
4. Test
$ forge test
RaymondFam (lookout) commented:
gzeon (judge) commented:
hieronx (Centrifuge) confirmed and commented:
