search

code-423n4 / 2021-06-gro-findings

0 stars 1 forks source link

Flash loan risk mitigation is optional and not robust enough #52

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The switchEoaOnly() allows the owner to disable preventSmartContracts (the project’s plan apparently is to do so after the beta-period) which will allow any smart contract to interact with the protocol and potentially exploit any underlying flash loan vulnerabilities which are specified as an area of critical concern.

The current mitigation is to optionally prevent contracts, except whitelisted partner ones, from interacting with the protocol to prevent any flash loan manipulations. A more robust approach is to add logic to prevent multiple txs to protocol from the same address/tx.origin within the same block when smart contracts are allowed. This will avoid any reliance on trust with integrating partners/protocols.

Proof of Concept

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/DepositHandler.sol#L112

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/WithdrawHandler.sol#L211

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L171-L174

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L176-L178

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L266-L272

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add logic to prevent multiple txs to protocol from the same address within the same block.

kitty-the-kat commented 3 years ago

Low-severity: This is a temporary blocker to not let SCs interact with gro-protocol, planned to be removed after beta as it might potentially stop other integrations (as per issue 51)

ghoul-sol commented 3 years ago

It looks like a low risk issue since it's a future problem and not something that is an immediate issue, however, it's not clear how the protocol will protect itself against flash loans after this temporary blocker is off. One of the critical protocol's concerns are flash loans manipulations therefore I think medium risk is justified here.