Critical address change

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L109
https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L129
https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L62

Vulnerability details

Impact

Changing critical addresses in contracts should be a two-step process. in order to prevent changing admin by mistake

Proof of Concept

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L109
https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L129
https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L62

check these links:
OpenZeppelin/openzeppelin-contracts#1488
OpenZeppelin/openzeppelin-contracts#2369

Tools Used

Manual

Recommended Mitigation Steps

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

[JTraversa]

This is a group of methods that are necessary as initial setters for our contracts.

Once set, they will be unnecessary to change, and this makes any "accidental" changes impossible given an incorrect setup would lead to no loss of funds, but just require a re-deployment.

That said, admin ability to do anything malicious could be set behind the same delay as our emergency admin withdrawal procedure to ensure users have a chance to review changes, and/or we could limit setters to initialization and require address(0)