The setMinter() function in Vcon.sol lacks both zero address checks and a proper ownership transfer pattern. I am submitting this as a medium-severity issue separate from similar low-severity instances due to this example's effect on the entire protocol. If the minter is set incorrectly due to human error or a malicious actor, VCON will not be able to be minted ever again.
Proof of Concept
The following code illustrates the setMinter() function.
function setMinter(address minter_) external {
require(
msg.sender == minter,
"Vcon: only the minter can change the minter address"
);
emit MinterChanged(minter, minter_);
minter = minter_;
}
It is clear that this function does not check that _minter != address(0). Similarly, if _minter is input incorrectly, either an unknown address or non-existent address will be set as the minter. Since only the minter can set a new minter, the minting functionality will be lost forever.
Tools Used
Manual review.
Recommended Mitigation Steps
A proper ownership transfer pattern would be helpful to eliminate all risks outlined above. The steps to achieve:
changeMinter() function sets a variable pendingNewMinter = _minter
acceptMinter() function must be called by pendingNewMinter for the minter to be fully set.
Lines of code
https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/vcon/Vcon.sol#L110-L117
Vulnerability details
Impact
The
setMinter()
function inVcon.sol
lacks both zero address checks and a proper ownership transfer pattern. I am submitting this as a medium-severity issue separate from similar low-severity instances due to this example's effect on the entire protocol. If the minter is set incorrectly due to human error or a malicious actor, VCON will not be able to be minted ever again.Proof of Concept
The following code illustrates the
setMinter()
function.It is clear that this function does not check that
_minter
!= address(0). Similarly, if_minter
is input incorrectly, either an unknown address or non-existent address will be set as the minter. Since only the minter can set a new minter, the minting functionality will be lost forever.Tools Used
Manual review.
Recommended Mitigation Steps
A proper ownership transfer pattern would be helpful to eliminate all risks outlined above. The steps to achieve:
changeMinter()
function sets a variablependingNewMinter = _minter
acceptMinter()
function must be called bypendingNewMinter
for theminter
to be fully set.