function setOperator(address _operator) public onlyOperator { operator = _operator; }
In the first case we can see a simple transfer of privileges. This one:
Doesn't check for 0 address
If mistakenly put the address, operator privileges will be lost and forced to be redeployed the contract in case it want to be recovered. This would lead to:
Some functions not being usable until redeployed
BorrowController.sol (allow(), deny(), setOperator())
This means that markets can't being denied in case they get compromised
And neither can be allowed new markets
Extra gas cost for redeploying
Reputational damage (most of the times more valuable than the previous one)
Doesn't emit an event that helps monitoring off-chain, this would make more probable the fact that
Basically this way is error prone, an not recommended.
As we can see in the other files, operator role is managed better, these files use a double step transfer process, where first pendingOperator is assigned, then it's claimed by the pending address:
Lines of code
https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/BorrowControl.sol#L1
Vulnerability details
operator
role it's handled in a way that can damage the protocolImpact
Same logic is being deployed several times, for maintainability is hardly recommended to not copy paste same logic many times but inherit it.
Also there is an issue in
BorrowController
with: https://github.com/code-423n4/2022-10-inverse/blob/aaf717f13c1f9a6e2f0888fef7c9b40400eeb4ec/src/BorrowController.sol#L26In the first case we can see a simple transfer of privileges. This one:
operator
privileges will be lost and forced to be redeployed the contract in case it want to be recovered. This would lead to:BorrowController.sol
(allow()
,deny()
,setOperator()
) This means that markets can't being denied in case they get compromised And neither can be allowed new marketsBasically this way is error prone, an not recommended.
As we can see in the other files, operator role is managed better, these files use a double step transfer process, where first
pendingOperator
is assigned, then it's claimed by the pending address:https://github.com/code-423n4/2022-10-inverse/blob/aaf717f13c1f9a6e2f0888fef7c9b40400eeb4ec/src/DBR.sol#L53 https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Oracle.sol#L44
In this case,
Details
All of these methods can be grouped into a single contract
Methods involved:
onlyOperator
setPendingOperator
claimOperator
setOperator
Params involved:
operator
pendingOperator
Events involved:
ChangeOperator
Github Permalinks
https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/DBR.sol# https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Oracle.sol# https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/BorrowControl.sol#
Mitigation
setOperator
in the operator role contract, in case this method is kept, add at least the event and the 0 check.