The Vault.sol contract has two address state variables, the keeper variable and the controller variable, which are both permitted to be the zero address. If both variables are zero simultaneously, any address can burn the available funds (available funds = balance - totalDebt) by sending these tokens to the zero address with the unprotected utilitize() function. If a user has no totalDebt, the user can lose their entire underlying token balance because of this.
Proof of Concept
The problematic utilize() function is found here. To see how the two preconditions can occur:
The keeper state variable is only changed by the setKeeper() function found here. If this function is not called, the keeper variable will retain the default value of address(0), which bypasses the only access control for the utilize function.
There is a comment here on line 69 stating the controller state variable can be zero. There is no zero address check for the controller state variable in the Vault constructor.
If both address variables are left at their defaults of address(0), then the safeTransfer() call on line 348 would send the tokens to address(0).
Recommended Mitigation Steps
Add the following line to the very beginning of the utilize() function:
require(address(controller) != address(0))
This check is already found in many other functions in Vault.sol, including the _unutilize() function.
oishun1112
commented
2 years ago
Handle
sirhashalot
Vulnerability details
Impact
The Vault.sol contract has two address state variables, the
keepervariable and thecontrollervariable, which are both permitted to be the zero address. If both variables are zero simultaneously, any address can burn the available funds (available funds = balance - totalDebt) by sending these tokens to the zero address with the unprotectedutilitize()function. If a user has no totalDebt, the user can lose their entire underlying token balance because of this.Proof of Concept
The problematic
utilize()function is found here. To see how the two preconditions can occur:setKeeper()function found here. If this function is not called, the keeper variable will retain the default value of address(0), which bypasses the only access control for the utilize function.If both address variables are left at their defaults of address(0), then the safeTransfer() call on line 348 would send the tokens to address(0).
Recommended Mitigation Steps
Add the following line to the very beginning of the
utilize()function:require(address(controller) != address(0))This check is already found in many other functions in Vault.sol, including the
_unutilize()function.