Missing reentrancy guard and contract existence check for modules

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

Protocol allows users to call registered modules via delegateCall in Module operation. It is not clear how these modules are validated before registration. If they are malicious they could cause reentrancy in the batch() call because there is no reentrancy guard protection. If they are destructed, delegateCall will still return success because low-level calls do not check for contract existence. Both will cause an undetermined level of impact to the protocol but the likelihood is low given the registration process and assumed validation there.

Proof of Concept

Eve manages to get her malicious module registered which causes reentrancy or maliciously affects protocol accounts/operations due to the delegateCall.

Alternatively, Alice registers a benign module but then accidentally calls selfDestruct on it. The module delegation is successful but without any side-effect because it doesn’t exist anymore.

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Ladle.sol#L239-L241

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Ladle.sol#L587-L595

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Ladle.sol#L96-L103

Tools Used

Manual Analysis

Recommended Mitigation Steps

1. Ensure during module registration that modules are trustworthy without any possibility to cause reentrancies or self-destruct.
2. Add reentrancy guard on batch() and contract existence check on module before delegateCall

[alcueca]

We know that using modules is inherently dangerous and their permissioning will be subject to the highest level of scrutiny.

[uivlis]

This ticket's fix requires no PR, so I will attach no link for the fix.