Enabling preventSmartContracts may lead to lock/loss of funds

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

preventSmartContracts is initialized to false which allows users to deposit/withdraw funds from the protocol via (custom) smart contracts because the eoaOnly check during deposits/withdrawals always succeeds. However, if protocol owner decides to suddenly enable preventSmartContracts then smart contracts are prevented from interaction unless they are exempted in safe addresses.

The lack of an event in switchEoaOnly() to inform off-chain monitors/interfaces about the enabling/disabling, say from false -> true, and lack of a time-delayed enforcement of this prevention of contracts from depositing/withdrawing causes users who have previously deposited via smart contracts (that are not safeAddresses) to get locked out of withdrawals leading to fund lock/loss.

Scenario: User deposits funds via smart contract (not in safe address list) when preventSmartContracts=false. Protocol owner sets preventSmartContracts=true. User’s funds are locked/lost in protocol.

Proof of Concept

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

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

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add event + time-delayed enforcement to switchEoaOnly() so users can monitor and withdraw funds deposited via smart contracts.

[kitty-the-kat]

Low criticality/Not an issue - Workaround exists (safe addresses)

• Owner will be a timelock

[ghoul-sol]

Agree with sponsor. While the scenario is correct, it all comes down to the management of the protocol. From different context I also assume that this option will be set to true for beta and safe addresses will be whitelisted. I'm making this a low risk because this can create too many angry users to be non-critical.