setAdmin does follow a top step process in setting a new admin
Summary
The setAdmin function is a delicate process. It could lead to loss of authorization to critical functions incase of typos or bad copy/paste. A two step process should be used as a guard against setting the wrong admin.
Vulnerability Detail
Critical change of important authorisation addresses should follow a two step process to prevent loss of access to certain functionality.
/// @notice sets the admin address
/// @param a Address of a new admin
/// @return bool true if successful
function setAdmin(address a) external authorized(admin) returns (bool) {
admin = a;
emit SetAdmin(a);
return true;
}
The setAdmin() function used in Redeemer, Lender and MarketPlace does not folow a two step process. If the wrong address is specificed by mistake, loss of the admin role would lead to permanent loss of access as there's no way to correct the error. The new admin should be added without first overwriting the previous one. Once this is done, the new admin can then remove the old one.
Impact
The admin is responsible for setting multiple critical operation such as the creation of markets, scheduling withdrawals and so son. Loss of the admin role would therefore lead to breaking of how the protocol works including loss of funds.
The new admin should be added without first overwriting the previous one. Once this is done, the new admin can then remove the old one. A check for the 0 address should also be added.
ck
high
setAdmin does follow a top step process in setting a new admin
Summary
The
setAdminfunction is a delicate process. It could lead to loss of authorization to critical functions incase of typos or bad copy/paste. A two step process should be used as a guard against setting the wrong admin.Vulnerability Detail
Critical change of important authorisation addresses should follow a two step process to prevent loss of access to certain functionality.
The
setAdmin()function used inRedeemer, Lender and MarketPlacedoes not folow a two step process. If the wrong address is specificed by mistake, loss of the admin role would lead to permanent loss of access as there's no way to correct the error. The new admin should be added without first overwriting the previous one. Once this is done, the new admin can then remove the old one.Impact
The admin is responsible for setting multiple critical operation such as the creation of markets, scheduling withdrawals and so son. Loss of the admin role would therefore lead to breaking of how the protocol works including loss of funds.
Code Snippet
https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/Lender.sol#L265-L272 https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/MarketPlace.sol#L273-L280 https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/Redeemer.sol#L129-L136
Tool used
Manual Review
Recommendation
The new admin should be added without first overwriting the previous one. Once this is done, the new admin can then remove the old one. A check for the 0 address should also be added.