AIP-144 Disclosure

Governance
1

TL;DR

Llama discovered an issue with the AIP-144 Swap Contract with respect to the aUST asset after the proposal was executed. There were ~$20k worth of funds at risk, which we whitehat-rescued. We also confirmed that this issue affected only aUST in the proposal scope and no other assets. Due to the rescue, Aave Protocol and Aave DAO will not bear any loss from this issue.

Overview

As part of AIP-144, Llama deployed a Swap contract that encouraged users to deposit USDC in order to receive certain long-tail assets along with a small premium from the Aave Ethereum v2 Collector Contract. This proposal was intended to consolidate the various Aave Treasury holdings into USDC.

Shortly after proposal execution, we noticed that a boolean value (ethFeedOnly) was set incorrectly in the Swap Contract constructor for the aUST configuration. This caused the contract to treat USD as ETH, which meant an arbitrager could acquire approximately $20k worth of aUST (i.e.~892,451 aUST) for ~12 USDC.

A Llama contributor, @Dydymoon, found this issue and took a whitehat action of arbitraging the entire aUST amount in this specific transaction. We have returned the entire aUST amount to the Aave collector (test transaction, final transaction).

We have cross-checked all remaining assets and confirmed that they are configured with the correct ethFeedOnly boolean value for their respective oracles. No other assets were affected.

Root Cause

The specific issue can be narrowed down to this boolean value being set to false instead of true at line 179 in the Swap Contract constructor.

That specific boolean (ethFeedOnly) tells the contract whether the oracle being used is a USD-denominated feed or an ETH-denominated feed for a particular asset. In this case, the aUST Oracle that was used was an ETH feed. However, with that boolean set as false, the contract assumed the oracle was a USD feed and didn’t execute this particular code block, which would have resulted in the appropriate price and decimal conversion from ETH to USD.

Next Steps

We will work on a separate proposal payload to swap the aUST from the Aave collector. Additionally, we will share a post-mortem that details how incorrect constructor configurations like this one will be prevented in the future.

9 Likes
2

Thanks for the detailed post about the issue, and @Dydymoon for your help!

Having proper tests would have helped to catch that error and prevent this issue. Would be nice to see the intention of improving your internal processes for code review and testing as “Next Steps”.

6 Likes
3

Thanks @miguelmtz. Yes, we plan to work on improving our processes and will include a more detailed plan in our post-mortem.

Sharing additional information here for the community’s benefit:

We write test suites for all our AIPs and try to be as comprehensive as possible, including end-to-end integration testing with Aave governance by forking mainnet. For AIP-144, you can find our test suite here. Here, we wrote tests to cover all branches of code logic within the contract to ensure that it’s working as expected. However, we missed testing all the initial constructor configurations for each individual asset. We did have a specific value test for BUSD and aSUSD that checked if the getAmountIn() function was working as intended for a specific value in the case of a USD-based feed and an ETH-based feed. Had we run this test for all assets in addition to writing separate tests for asserting the correct initial constructor configurations, we would have been able to catch the bug. We are actively working to improve in our internal process to implement more comprehensive tests and also to ensure that these types of bugs are caught during code review.

4 Likes
4

Thanks, @Llamaxyz for this disclosure. From our side, we have analyzed what went wrong and what could be a good way forward for us to help teams submit more safely proposals.

  • We think that our reviewing process could be improved by giving early feedback here on the forum, if we feel that the general design/approach of the proposal could be more optimal, a similar approach as here [ARFC] Repay Excess CRV Debt on Ethereum v2 - #7 by bgdlabs, so we will proceed that way.

  • We will improve the tooling of the community (e.g. Seatbelt) to go a bit deeper on the verification of scenarios like this one (assets management), which are becoming more common.

  • We will coordinate with the security partners of the community to understand if it is better to approach differently to smart contracts of a similar style to the ones on the 144 system.

  • On our reviews, we will be stricter on the test cases required, whenever applicable.

8 Likes
5

We conducted an internal post-mortem recently and have agreed to improve the following in our internal processes to ensure such bugs are caught well in advance:

Additional testing requirements as part of development:

  1. Adding specific value tests for every configuration in a contract (Besides generic unit tests of the respective code block) to check that all configurations are working as intended.
  2. Adding situational integration tests post-execution of a function in a contract to catch any unintended behavior post-execution of a function.
  3. Adding assertions for every major state change within a code block and not just the input/output of a code block, in order to ensure that state changes are occurring as intended.

We will be planning our proposal pipeline such that there is at least a month of advance notice for each proposal to the Llama Engineering team in order to ensure that there is sufficient time for development, testing, internal review and external review.

5 Likes