Platform: Code4rena
Start Date: 27/05/2021
Pot Size: $100,000 USDC
Total HM: 12
Participants: 7
Period: 7 days
Judge: LSDan
Total Solo HM: 10
Id: 12
League: ETH
Rank: 2/7
Findings: 4
Award: $24,201.69
🌟 Selected for report: 8
🚀 Solo Findings: 3
🌟 Selected for report: 0xRajeev
4295.5942 USDC - $4,295.59
0xRajeev
The vaultID for a new vault being built is required to be specified by the user building a vault via the build() function (instead of being assigned by the Cauldron/protocol). An attacker can observe a build() as part of a batch transaction in the mempool, identify the vaultID being requested and front-run that by constructing a malicious batch transaction with only the build operation with that same vaultID. The protocol would create a vault with that vaultID and assign attacker as its owner. More importantly, the valid batch transaction in the mempool which was front-run will later fail to create its vault because that vaultID already exists, as per the check on Line180 of Cauldron.sol. As a result, the valid batch transaction fails entirely because of the attacker front-running with the observed vaultID.
While the attacker gains nothing except the ownership of an empty vault after spending the gas, this could grief the protocol’s real users by preventing them from opening a vault and interacting with the protocol in any manner.
Rationale for Medium-severity impact: While the likelihood of this may be low, the impact is high because valid vaults from the Yield front-end will never be successfully created and will lead to a DoS against the entire protocol’s functioning. So, with low likelihood and high impact, the severity (according to OWASP) is medium.
Alice uses Yield’s front-end to create a valid batch transaction. Evil Eve observes that in the mempool and identifies the vaultID of the vault being built by Alice. Eve submits her own batch transaction (without using the front-end) with only a build operation using Alice’s vaultID. She uses a higher gas price to front-run Alice’s transaction and get’s the protocol to assign that vaultID to herself. Alice’s batch transaction later fails because the vaultID she requested is already assigned to Eve. Eve can do this for any valid transaction to grief protocol users by wasting her gas to cause DoS.
Manual Analysis
Mitigate this DoS vector by having the Cauldron assign the vauldID instead of user specifying it in the build() operation. This would likely require the build() to be a separate non-batch transaction followed by other operations that use the vaultID assigned in build(). Consider the pros/cons of this approach because it will significantly affect the batching/caching logic in Ladle.
Alternatively, consider adding validation logic in Ladle’s batching to revert batches that have only build or a subset of the operations that do not make sense to the protocol’s operations per valid recipes, which could be an attacker’s signature pattern.
#0 - alcueca
2021-06-02T20:41:17Z
Good find.
#1 - alcueca
2021-06-04T09:59:25Z
🌟 Selected for report: 0xRajeev
4295.5942 USDC - $4,295.59
0xRajeev
The grab() function in Cauldron is used by the Witch or other liquidation engines to grab vaults that are under-collateralized. To prevent re-grabbing without sufficient time for auctioning collateral/debt, the logic uses an auctionInterval threshold to give a reasonable window to a liquidation engine that has grabbed the vault.
The grab() function has a comment on Line 354: “// Grabbing a vault protects it for a day from being grabbed by another liquidator. All grabbed vaults will be suddenly released on the 7th of February 2106, at 06:28:16 GMT. I can live with that.” indicating a requirement of the auctionInterval being equal to one day. This can happen only if the auctionInterval is set appropriately. However, this state variable is uninitialized (defaults to 0) and depends on setAuctionInterval() being called with the appropriate auctionInterval_ value which is also not validated.
Discussion with the project lead indicated that this comment is incorrect. Nevertheless, it is safer to initialize auctionInterval at declaration to a safe default value instead of the current 0 which will allow liquidation engines to re-grab vaults without making any progress on liquidation auction. It is also good to add a threshold check in setAuctionInterval() to ensure the new value meets/exceeds a reasonable default value.
Rationale for Medium-severity impact: While the likelihood of this may be low, the impact is high because liquidation engines will keep re-grabbing vaults from each other and potentially result in liquidation bots entering a live-lock situation without making any progress on liquidation auctions. This will result in collateral being stuck and impact entire protocol’s functioning. So, with low likelihood and high impact, the severity (according to OWASP) is medium.
Configuration recipe forgets to set the auctionInterval state variable by calling setAuctionInterval() and inadvertently leaves it at the default value of 0. Alternatively, it calls it but with a lower than intended/reasonable auction interval value. Both scenarios fail to give sufficient protection to liquidation engines from having their grabbed vaults re-grabbed without sufficient time for liquidation auctions.
Manual Analysis
#0 - alcueca
2021-06-02T20:50:56Z
While we support multiple liquidation engines, we are going live with only one, and only one is in scope of the contest.
Zero is an acceptable value for auctionInterval of using just one liquidation engine (the Witch), and the issue can be solved with better natspec and better documentation, therefore a severity of 0 is suggested.
#1 - alcueca
2021-06-03T01:36:36Z
Actually, if we forget to set auctionInterval to a reasonable value, an attacker could repeatedly call grab with just one Witch, stopping auctions from dropping down in price until an emergency governance action is taken.
Therefore, I would suggest the severity to be reduced to 1 instead of zero.
🌟 Selected for report: 0xRajeev
4295.5942 USDC - $4,295.59
0xRajeev
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.
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.
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.
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
Manual Analysis
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.
#0 - alcueca
2021-06-02T20:46:16Z
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.
#1 - dmvt
2021-06-14T20:14:56Z
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.
#2 - alcueca
2021-06-16T14:17:14Z
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.
644.3391 USDC - $644.34
0xRajeev
setDebtLimits() is used to set the maximum and minimum debt for an underlying and ilk pair. The assumption is that max will be greater than min while setting them because otherwise the debt checks in _pour() for line/dust will fail and revert.
While max and min debt limits can be reset, it is safer to perform input validation on them in setDebtLimits().
A recipe incorrectly interchanges the values of min and max debt which leads to exceptions in pouring into the vaults.
Manual Analysis
Add a check to ensure max > mix.
#0 - alcueca
2021-06-02T21:03:47Z
Duplicate with #22
🌟 Selected for report: 0xRajeev
1431.8647 USDC - $1,431.86
0xRajeev
Many batched operation functions return values but these are ignored by the caller batch(). While this may be acceptable for the front-end which picks up any state changes from such functions via emitted events, integrating protocols that make a call to batch() may require it to package and send back return values of all operations from the batch to react on-chain to the success/failure or other return values from such calls. Otherwise, they will be in the dark on the success/impact of batched operations they’ve triggered.
Manual Analysis
Package and send back return values of all batched operations’ functions to the caller of batch().
🌟 Selected for report: 0xRajeev
1431.8647 USDC - $1,431.86
0xRajeev
Add an address check in receive() of Ladle.sol to ensure the only address sending ETH being received in receive() is the Weth9 contract (similar to the check in PoolRouter.sol) for Ether withdrawal in _exitEther().
This will prevent stray Ether from being sent accidentally to this contract and getting locked.
Manual Analysis
Add an address check in receive() of Ladle.sol to ensure only Weth9 contract can send Ether to this contract.
🌟 Selected for report: 0xRajeev
1431.8647 USDC - $1,431.86
0xRajeev
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.
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.
Manual Analysis
#0 - alcueca
2021-06-02T20:36:35Z
We know that using modules is inherently dangerous and their permissioning will be subject to the highest level of scrutiny.
#1 - uivlis
2021-06-22T09:21:24Z
This ticket's fix requires no PR, so I will attach no link for the fix.
🌟 Selected for report: 0xRajeev
1431.8647 USDC - $1,431.86
0xRajeev
The LOCK role is special in AccessControl because it has itself as the admin role (like ROOT) but no members. This means that calling setRoleAdmin(msg.sig, LOCK) means no one can grant/revoke that msg.sig role anymore and it gets locked irreversibly. This means it disables admin-based permissioning management of that role and therefore is very powerful in its impact.
Given this, there is a special function lockRole() which is specifically meant to enforce LOCK as the admin for the specified role parameter. For all other role admin creations, the generic setRoleAdmin() may be used. However, setRoleAdmin() does not itself prevent specifying the use of LOCK as the admin. If this is accidentally used then it leads to disabling that role’s admin management irreversibly similar to the lockRole() function.
It is safer to force admins to use lockRole() as the only way to set admin to LOCK and prevent the use of LOCK as the adminRole parameter in setRoleAdmin(), because doing so will make the intention of the caller clearer as lockRole() clearly has that functionality specified in its name and that’s the only thing it does.
Alice who is the admin for foo() wants to give the admin rights to Bob (0xFFFFFFF0) but instead of calling setRoleAdmin(foo.sig, 0xFFFFFFF0), she calls setRoleAdmin(foo.sig, 0xFFFFFFFF) where 0xFFFFFFFF is LOCK. This makes LOCK as the admin for foo() and prevents any further admin-based access control management for foo().
Manual Analysis
Prevent the use of LOCK as the adminRole parameter in setRoleAdmin().