🍢 🔒 KebabSec Audit 🔒 🍢

[ControlCplusControlV]

High

• [x] #189
• [x] #190

Medium

• [x] #192
• [x] #191
• [x] #193
• [x] #194

Low

• [x] #195
• [x] #196
• [x] #197
• [x] #198
• [x] #199
• [x] #200
• [x] #202
• [x] #201
• [x] #203

Informational

• [x] #214
• [x] #213
• [x] #212
• [x] #211
• [x] #210
• [x] #209
• [x] #208
• [x] #207
• [x] #206
• [x] #205
• [x] #204

[Shungy]

I think fix to https://github.com/manifoldfinance/mevETH2/issues/192 introduced a DOS vector. It now allows 1 wei of transfers to update lastUpdate, hence preventing withdrawals for a targeted user. Working internally to confirm this.

[okkothejawa]

I think fix to #192 introduced a DOS vector. It now allows 1 wei of transfers to update lastUpdate, hence preventing withdrawals for a targeted user. Working internally to confirm this.

Agreed on this as the transfer/transferFrom to the victim can be combined with grantRewards beforehand the victim's withdraw action to cause DOS. Same can be done with depositing to victim, albeit more expensive due to the 0.01 ether limit, but it still constitutes a valid risk imo. Sandwich bypass issue should be resolved in another way.

I believe it can be mitigated to a degree by locking grantRewards as then the DOS can only happen if the victim wants to withdraw in the same block with an authorized reward distribution, which creates another case for locking grantRewards I think.

[ControlCplusControlV]

I think fix to #192 introduced a DOS vector. It now allows 1 wei of transfers to update lastUpdate, hence preventing withdrawals for a targeted user. Working internally to confirm this.

Agreed on this as the transfer/transferFrom to the victim can be combined with grantRewards beforehand the victim's withdraw action to cause DOS. Same can be done with depositing to victim, albeit more expensive due to the 0.01 ether limit, but it still constitutes a valid risk imo. Sandwich bypass issue should be resolved in another way.

I believe it can be mitigated to a degree by locking grantRewards as then the DOS can only happen if the victim wants to withdraw in the same block with an authorized reward distribution, which creates another case for locking grantRewards I think.

https://github.com/manifoldfinance/mevETH2/pull/260/commits/488ad3a6826530478bcbe0ddf13a26b69ef1067c I've locked down grantRewards, do you think this would be sufficient to stop a potential DOS? or would you still prefer Sandwhich bypass being resolved in another way

[sambacha]

There should be no atomic grant Rewards anyway: this is another variation of MasterChef exploit

[okkothejawa]

I think fix to #192 introduced a DOS vector. It now allows 1 wei of transfers to update lastUpdate, hence preventing withdrawals for a targeted user. Working internally to confirm this.

Agreed on this as the transfer/transferFrom to the victim can be combined with grantRewards beforehand the victim's withdraw action to cause DOS. Same can be done with depositing to victim, albeit more expensive due to the 0.01 ether limit, but it still constitutes a valid risk imo. Sandwich bypass issue should be resolved in another way. I believe it can be mitigated to a degree by locking grantRewards as then the DOS can only happen if the victim wants to withdraw in the same block with an authorized reward distribution, which creates another case for locking grantRewards I think.

488ad3a I've locked down grantRewards, do you think this would be sufficient to stop a potential DOS? or would you still prefer Sandwhich bypass being resolved in another way

It is not sufficient in theory, but should be good enough imo. Because to DOS a withdraw, it should be included in the same block with a rewards call and the attacker should be arrange them so that they would be included in the same block and the rewards call would precede the withdraw action. But it doesn't mitigate the actual DOS vector, which is that it is possible for an attacker to change lastDeposit of an arbitrary user by utilizing transfer/transferFrom or by choosing them as the receiver of a deposit action (which is at least 0.01 ether so it is a bad DOS but it's still DOS). If there is a good way of resolving the actual DOS vector, thinking a bit on it might be very nice, but I don't it's an absolute necessity as the attacker cannot trigger rewards call at will after the locking of grantRewards, its virtually not useful in practice for the attacker.

[sambacha]

Sounds reasonable. This isn't a vulnerability that affects user deposits, so its an acceptable risk as it puts the burden on operations not users.

Inshallah.