Here's an exhaustive list of all issues/notes with current status
1) GelatoUserProxy.callAction and GelatoUserProxy.delegatecallAction is supposed to be internal
FIXED
2) GelatoCore:line 270 ( gasleft() - _internalGasRequirement ) can underflow, because there's some gas consumed since require(startGas > _internalGasRequirement, "GelatoCore.exec: Insufficient gas sent")
RECOMMENDATION: Probably should be addressed by replacing this require
require(startGas > _internalGasRequirement, "GelatoCore.exec: Insufficient gas sent");
by a require that checks that gasleft() > _internalGasRequirement just before this.executionWrapper is called, or even better just using safemath in this line:
3) GelatoCore.sol:line 166 sideffect of providerCanExec not being called on tasks submitted by the provider is that no taskSpecGasPriceCeil can be specified for these tasks.
OPEN ISSUE: probably not a big deal, but it is a flaw
4) GelatoCore.sol:line 202 might be useful to include _TR.task().actions[i].value in the termsOk call?
FIXED
5) I think taskReceiptHash[_TR.id] in the exec function should be deleted before executionWrapper call and restored after if necessary. Otherwise you open yourself to re-entrancy attacks from the executor, if he finds a way to set up some callback to their contract downstream from the call being made.
FIXED
6) I was thinking that there's probably no strict requirement to submit TaskSpec to provideTaskSpecs or unprovideTaskSpecs as opposed to just hash. Which would be cheaper and some providers could prefer not revealing the spec until a task is being executed.
NOT AN ISSUE
7) In _processProviderPayables() safemath exception can happen if startGas is much higher than _gelatoMaxGas
because estExecTxGas is capped to _gelatoMaxGas (which is constant) and gasleft() can be basically arbitrarily high.
RECOMMENDATION: This should probably be fixed by capping estGasConsumed to _gelatoMaxGas + EXEC_TX_OVERHEAD instead of capping estExecTxGas
8) I also wonder about the logic behind the minExecutorStake, since in practice, executors can unstake at any moment, I'm not sure it works as a detractor of bad behavior. I also don't know what to think about the fact that executors can be assigned providers by other executors against their will, since they can pass them on to another executor it's more of a nuisance, but still probably not correct.
OPEN ISSUE: probably requires further discussion
9) Do you think having one global min gas requirement ( gelatoMaxGas ) for refunded reverts will work for all providers?
What I meant is that right now, if executors want to be protected from reverts that are not their fault, they always have to provide gas that is close to block gas limit. It might be useful if providers could specify lower gas limit in their task spec, which would also protect them from executors intentionally inflating gas costs by hooking to some call in the task.
OPEN ISSUE: probably requires further discussion
10) If _TR.userProxy == _TR.provider.addr canExec will not check providerModuleChecks which is imo incorrect and also not consistent with canSubmitTask
ADDRESSED
11) Also, I think that purely from security perspective, canSubmitTask function is not necessary, since it performs the same checks that will be performed by canExec (with the exception of isExecutorMinStaked and the issue I've mentioned above). It might be argued it's a spam protection, but the spam it prevents is more expensive for the spammer than the target of the spam (executor).
OPEN ISSUE:
Hilmar: "well the goal is that gelato will be used, similiar to gas station network on multiple UIs that offer individual automation use cases. Hence we would not have direct control over that, only through providing front end libraries that call canSubmitTask under the hood. Will think about it."
12) Btw. checking executor minStake also happens only on task submission and not on execution, it would make sense the oth
er way around to me
OPEN ISSUE: maybe requires further discussion, but probably fine
13) There's general question of how to open executor market and incentivise executors to submit tasks quickly.
OPEN ISSUE: won't be solved in this release
14) Isn't ActionWithdrawBatchExchange.action supposed to call FeeFinder.redeemCredit?
WILL BE FIXED
15) in ActionPlaceOrderBatchExchangePayFee.action , _order.sellAmount will underflow if _order.sellAmount is smaller than fee
WILL BE FIXED
16) I think in FeeExtractor.payFee, the require should be:
getFeeAmount(_feeToken) <= _feeAmount
and the transfer should be:
IERC20(_feeToken).safeTransferFrom(msg.sender, provider, getFeeAmount(_feeToken))
because it would probably be hard to hit the exact _feeAmount if it's derived from current exchange price of the token
WILL BE FIXED
17) But also, I think the way fee is implemented in ActionPlaceOrderBatchExchangePayFee.action is potentially quite dangerous, because of the possibility of atomic exchange price manipulation that might massively inflate the fee. I think maxFee should be part of _order. At the same time, exchange price manipulation could be used to reduce the fee, but I think that's not very likely since the benefit of that is very limited.
WILL BE FIXED
18) In ProviderModuleGnosisSafeProxy.isGelatoCoreWhitelisted it's much better to use GnosisSafe.isModuleEnabled than GnosisSafe.getModules to check whether the module is enabled. Especially since one is O(n) and the other O(1)
NOT A CRITICAL ISSUE, BUT MIGHT BE FIXED:
Hilmar: What we could do:
a) Read the masterCopy address of the userProxy
b) If it's the new masterCopy with the new modier, call the isModuleFunc
c) Else, do the old loop
but the good thing about providerModules is that they are modular and can be replaced by providers easily. So we can always say we upgraded it and made it more efficent and provider can choose to provide it
Here's an exhaustive list of all issues/notes with current status
1) GelatoUserProxy.callAction and GelatoUserProxy.delegatecallAction is supposed to be internal
FIXED
2) GelatoCore:line 270 ( gasleft() - _internalGasRequirement ) can underflow, because there's some gas consumed since require(startGas > _internalGasRequirement, "GelatoCore.exec: Insufficient gas sent")
RECOMMENDATION: Probably should be addressed by replacing this require
require(startGas > _internalGasRequirement, "GelatoCore.exec: Insufficient gas sent");
by a require that checks that gasleft() > _internalGasRequirement just before this.executionWrapper is called, or even better just using safemath in this line:
try this.executionWrapper{gas: gasleft() - _internalGasRequirement}(
3) GelatoCore.sol:line 166 sideffect of providerCanExec not being called on tasks submitted by the provider is that no taskSpecGasPriceCeil can be specified for these tasks.
OPEN ISSUE: probably not a big deal, but it is a flaw
4) GelatoCore.sol:line 202 might be useful to include _TR.task().actions[i].value in the termsOk call?
FIXED
5) I think taskReceiptHash[_TR.id] in the exec function should be deleted before executionWrapper call and restored after if necessary. Otherwise you open yourself to re-entrancy attacks from the executor, if he finds a way to set up some callback to their contract downstream from the call being made.
FIXED
6) I was thinking that there's probably no strict requirement to submit TaskSpec to provideTaskSpecs or unprovideTaskSpecs as opposed to just hash. Which would be cheaper and some providers could prefer not revealing the spec until a task is being executed.
NOT AN ISSUE
7) In _processProviderPayables() safemath exception can happen if startGas is much higher than _gelatoMaxGas because estExecTxGas is capped to _gelatoMaxGas (which is constant) and gasleft() can be basically arbitrarily high.
RECOMMENDATION: This should probably be fixed by capping estGasConsumed to _gelatoMaxGas + EXEC_TX_OVERHEAD instead of capping estExecTxGas
8) I also wonder about the logic behind the minExecutorStake, since in practice, executors can unstake at any moment, I'm not sure it works as a detractor of bad behavior. I also don't know what to think about the fact that executors can be assigned providers by other executors against their will, since they can pass them on to another executor it's more of a nuisance, but still probably not correct.
OPEN ISSUE: probably requires further discussion
9) Do you think having one global min gas requirement ( gelatoMaxGas ) for refunded reverts will work for all providers? What I meant is that right now, if executors want to be protected from reverts that are not their fault, they always have to provide gas that is close to block gas limit. It might be useful if providers could specify lower gas limit in their task spec, which would also protect them from executors intentionally inflating gas costs by hooking to some call in the task.
OPEN ISSUE: probably requires further discussion
10) If _TR.userProxy == _TR.provider.addr canExec will not check providerModuleChecks which is imo incorrect and also not consistent with canSubmitTask
ADDRESSED
11) Also, I think that purely from security perspective, canSubmitTask function is not necessary, since it performs the same checks that will be performed by canExec (with the exception of isExecutorMinStaked and the issue I've mentioned above). It might be argued it's a spam protection, but the spam it prevents is more expensive for the spammer than the target of the spam (executor).
OPEN ISSUE: Hilmar: "well the goal is that gelato will be used, similiar to gas station network on multiple UIs that offer individual automation use cases. Hence we would not have direct control over that, only through providing front end libraries that call canSubmitTask under the hood. Will think about it."
12) Btw. checking executor minStake also happens only on task submission and not on execution, it would make sense the oth er way around to me
OPEN ISSUE: maybe requires further discussion, but probably fine
13) There's general question of how to open executor market and incentivise executors to submit tasks quickly.
OPEN ISSUE: won't be solved in this release
14) Isn't ActionWithdrawBatchExchange.action supposed to call FeeFinder.redeemCredit?
WILL BE FIXED
15) in ActionPlaceOrderBatchExchangePayFee.action , _order.sellAmount will underflow if _order.sellAmount is smaller than fee
WILL BE FIXED
16) I think in FeeExtractor.payFee, the require should be: getFeeAmount(_feeToken) <= _feeAmount and the transfer should be: IERC20(_feeToken).safeTransferFrom(msg.sender, provider, getFeeAmount(_feeToken)) because it would probably be hard to hit the exact _feeAmount if it's derived from current exchange price of the token
WILL BE FIXED
17) But also, I think the way fee is implemented in ActionPlaceOrderBatchExchangePayFee.action is potentially quite dangerous, because of the possibility of atomic exchange price manipulation that might massively inflate the fee. I think maxFee should be part of _order. At the same time, exchange price manipulation could be used to reduce the fee, but I think that's not very likely since the benefit of that is very limited.
WILL BE FIXED
18) In ProviderModuleGnosisSafeProxy.isGelatoCoreWhitelisted it's much better to use GnosisSafe.isModuleEnabled than GnosisSafe.getModules to check whether the module is enabled. Especially since one is O(n) and the other O(1)
NOT A CRITICAL ISSUE, BUT MIGHT BE FIXED: Hilmar: What we could do:
a) Read the masterCopy address of the userProxy b) If it's the new masterCopy with the new modier, call the isModuleFunc c) Else, do the old loop but the good thing about providerModules is that they are modular and can be replaced by providers easily. So we can always say we upgraded it and made it more efficent and provider can choose to provide it