The automation is the scariest feature of this contract. It might be a smarter design choice to put automation into a deployment script outside of the smart contract whenever possible.
For instance in setVaultLimits()
It is probably unnecessary to include:
if (address(parentVaultController) != 0) {
parentVaultController.topUpVault();
sendBackOverflow();
}
This could easily be done as a script run when a user uses the UI to set the limits as a second function call, it might even save gas by moving some calculations outside of the smart contract.
But the main issue is security; topUpVault() is called way too many times all over this contract. The example above is just the most obvious one that seemed out of place. It is a relatively dangerous function to call so liberally.
I do appreciate that this could impact user experience (multiple function calls could mean, having to click metamask's accept button multiple times) but that is why we can debate the merits of this idea below :-D
The automation is the scariest feature of this contract. It might be a smarter design choice to put automation into a deployment script outside of the smart contract whenever possible.
For instance in
setVaultLimits()It is probably unnecessary to include:
This could easily be done as a script run when a user uses the UI to set the limits as a second function call, it might even save gas by moving some calculations outside of the smart contract.
But the main issue is security;
topUpVault()is called way too many times all over this contract. The example above is just the most obvious one that seemed out of place. It is a relatively dangerous function to call so liberally.I do appreciate that this could impact user experience (multiple function calls could mean, having to click metamask's accept button multiple times) but that is why we can debate the merits of this idea below :-D