Violation of implicit constraints in batched operations may break protocol assumptions

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

The Ladle batching of operations is a complex task (as noted by the project lead) which has implicit constraints on what operations can be bundled together in a batch, which operations can/have-to appear how many times and in what order/sequence etc. Some examples of these constraints are: Join Ether should be the first operation, Exit Ether the last, and only one Join Ether per batch.

All this complexity is managed currently by anticipating all interactions to happen via their authorised front-end which uses validated (and currently revealed only on demand) recipes that adhere to these constraints. There is a plan to open the design up to other front-ends and partner integrating protocols who will also test their batch recipes or integrations for these constraints.

Breaking some of these constraints opens up the protocol to failing transactions, undefined behaviour or potentially loss/lock of funds. Defensive programming suggests enforcing such batch operation constraints in the code itself along with documentation and onboarding checks for defense-in-depth. Relying on documentation or external validation may not be sufficient for arguably the most critical aspect of batched operations which is the only authorized way to interact with the protocol.

Rationale for Medium-severity impact: While the likelihood of this may be low because of controlled/validated onboarding on new front-ends or integrating protocols, the impact of accidental deviation from implicit constraints is high. This may result in transaction failing or tokens getting locked/lost, thus impact the entire protocol’s functioning. So, with low likelihood and high impact, the severity (according to OWASP) is medium.

Proof of Concept

1) A new front-end project comes up claiming to provide a better user-interface than the project’s authorised front-end. It does not use the recipe book (correctly) and ends up making Ladle batches with incorrect operations which fail the constraints leading to protocol failures and token lock/loss.

2) An integrating protocol goes through the approved onboarding and validation but has missed bugs in its recipe for batches which fail the constraints leading to protocol failures and token lock/loss.

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

Comment on Join Ether constraint: https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Ladle.sol#L525

Comment on Exit Ether constraint: https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Ladle.sol#L538

Tools Used

Manual Analysis

Recommended Mitigation Steps

Enforce batch operation constraints explicitly in the code (e.g. with tracking counters/booleans for operations) along with documentation and onboarding validation. This may increase the complexity of the batching code but adds fail-safe defense-in-depth for any mistakes in onboarding validation of implicit constraints which may affect protocol operations significantly.

[alcueca]

I don't think implementing the suggested checks is worth the added complexity and deployment gas.

Given that this is an issue disclosed by the sponsor and widely discussed, I would find it unfair to the other wardens if accepted.

[dmvt]

The warden brings up a valid concern. As to the sponsor's objection, the sponsor has written "The Ladle is a complex contract, with wide-ranging powers over the protocol. It is the riskiest contract, and at the same time the one that has more room for a bug to hide. It is of the highest importance to find as many bugs as we can in it." (emphasis mine). I highly recommend the issue be addressed by the sponsor.

[alcueca]

I still disagree. I disclosed this issue myself first in the C4 discord, and it is a design choice.

Users are not expected to build batches themselves and won't be provided with any tools to do so. If an user decides to ignore all advice and go to the extreme length of interacting with the smart contracts himself, or via an unauthorised frontend, it's completely on him if he loses funds.

It is impossible that an user will execute a non-reverting batch without careful research on the batching system. To do that he would need to exactly match one or more action codes with abi-encoded arrays of parameters of the right types.

My concerns on the batching system are not on users or integrators building a bad batch and losing their funds.

My concerns are that the batching system is complex and there could be the chance that a batch could be built with a negative effect for the protocol or for other users. Since the Ladle has sweeping powers over the protocol, that would be a real issue. Such a batch has not been found.

So, to reiterate. The issue being brought up was brought up in the discord by myself, and it is a conscious trade off we made of usability for flexibility. No undisclosed issue has been found.