Submitted on Tue Aug 06 2024 02:44:31 GMT-0400 (Atlantic Standard Time) by @Ironside_Sec for Boost | Folks Finance
Report ID: #34153
Report type: Smart Contract
Report severity: Low
Target: https://testnet.snowtrace.io/address/0xA758c321DF6Cd949A8E074B22362a4366DB1b725
Impacts:
- Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Imagine if there's 10 rounds in 30 min TWAP period, if 3rd round query fails (due to unavailability of price update on that round id), then catch triggeres breaking the while loop, and 3 rounds price sum / 3 will be used as TWAP price. Instead, it should skip that round in catch and move to the next round to query so in the end sum of 9 rounds / 9 will be the TWAP price.
Since the Chainlink node is a library used inside node manager, the asset in scope chosen is Node manager
https://docs.chain.link/data-feeds/historical-data#roundid-in-aggregator-aggregatorroundid
Data feeds are updated in rounds. Rounds are identified by their roundId, which increases with each new round. This increase may not be monotonic. Knowing the roundId of a previous round allows contracts to consume historical data.
Chainlink says that round ids can be consumed to get previous prices history, but round ids are not increased monotonically. But in chainlnk node library, the price is queried monotonically in line 70 below --latestRoundId. The issue here is, the try will fail and trigger the catch if there's no price data in that round. If catch triggers then the loop will break making the TWAP price legit. Currently pools with chainlink node deesn't have parent, but other nodes have chainlink as parent and give chainlink for first priority. In this case, if TWAP price is considered stronger than all and used as fallback, the the attacker will be able to liquidate / repay / deposit at a price away from current legit price.
ChainlinkNode.sol
57: function getTWAPPrice(
58: AggregatorV3Interface chainlink,
59: uint80 latestRoundId,
60: uint256 latestPrice,
61: uint256 TWAPTimeInterval
62: ) internal view returns (uint256 price) {
63: uint256 priceSum = latestPrice;
64: uint256 priceCount = 1;
65:
66: uint256 startTime = block.timestamp - TWAPTimeInterval;
67:
69: while (latestRoundId > 0) {
70: >>> try chainlink.getRoundData(--latestRoundId) returns (
71: uint80,
72: int256 answer,
73: uint256,
74: uint256 updatedAt,
75: uint80
76: ) {
77: if (updatedAt < startTime) {
78: break;
79: }
80: priceSum += answer.toUint256();
81: priceCount++;
82: } catch {
83: >>> break;
84: }
85: }
86:
87: return priceSum / priceCount;
88: }
TWAP query stops the loop for the whole TWAP duration and still considers it as legit price and this price will be used by the hub pools in determining at what price to deposit/borrow/repay/liquidate.
https://docs.chain.link/data-feeds/historical-data#roundid-in-aggregator-aggregatorroundid
Since the Chainlink node is a library used inside node manager, the asset in scope choosen is Node manager
The POC shows the oracle node querying the TWAP on multiple rounds, but to find the round which was not monotonically rised is somewhere in the past and could not be found, so my poc just shows the querying past and if the round is not there then, it will revert with error No data present.
Also this POC doesn't show exactly how to repay at less price, because current chainlink node has 0 twap duration in LINK pool. So, its a showcase what happens in this reverting case. Also check the attached images
for POC to work,
- on
https://github.com/Folks-Finance/folks-finance-xchain-contractsdirectory, doforge i foundry-rs/forge-std --no-commit, - then add
ds-test/=node_modules/ds-test/toremappings.txt, - then create a file
Foundry.t.solon test/ dirctory. - Then run the poc with
forge t --mt testIssue -f https://rpc.ankr.com/avalanche_fuji -vvvv
// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "../contracts/oracle/modules/NodeManager.sol"; import "../contracts/oracle/interfaces/INodeManager.sol"; import "../contracts/oracle/storage/NodeDefinition.sol"; import "../contracts/oracle/storage/NodeOutput.sol"; import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";
contract PoC is Test { address from = 0xD5fba05dE4b2d303D03052e8aFbF31a767Bd908e; bytes32 accountId = 0xd32cc9b5264dc39d42622492da52b2b8100e6444367e20c9693ce28fe71286be;
NodeManager constant nodeManager = NodeManager(0xA758c321DF6Cd949A8E074B22362a4366DB1b725);
function setUp() public {}
function testIssue() public {
// https://testnet.snowtrace.io/tx/0x452af7f5cb38378ff97a20811a89596f381935060d49686f96de33911fc30b76?chainid=43113
bytes memory parameters = abi.encode(address(0x34C4c526902d88a3Aa98DB8a9b802603EB1E3470), 2 hours, 8);
bytes32[] memory parents;
bytes32 nodeId = nodeManager.registerNode(NodeDefinition.NodeType.CHAINLINK, parameters, parents);
NodeOutput.Data memory node = nodeManager.process(nodeId);
AggregatorV3Interface chainlinkAggregator = AggregatorV3Interface(0x34C4c526902d88a3Aa98DB8a9b802603EB1E3470);
vm.expectRevert();
(uint80 roundId, int256 answer, , uint256 updatedAt, ) = chainlinkAggregator.getRoundData(19446744073709595053);
}
}