Summary
The configureMarginCollateral function currently lacks validation for the priceFeedHeartbeatSeconds parameter, which could lead to several issues, including stale price data, market manipulation, system reliability concerns, and regulatory non-compliance.
In the current implementation, the function accepts any value for priceFeedHeartbeatSeconds without performing any checks on its magnitude. This includes accepting zero or arbitrarily large values. The heartbeat duration is crucial as it determines how long a price feed update is considered valid. An incorrectly set heartbeat can lead to the system operating with stale or manipulated price data.
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/branches/GlobalConfigurationBranch.sol#L243C4-L267C6
Proof of concept
Coded Proof of Concept
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "../src/GlobalConfigurationBranch.sol";
contract HeartbeatExploitTest is Test {
GlobalConfigurationBranch public configBranch;
address public owner = address(1);
address public attacker = address(2);
address public mockERC20 = address(3);
address public mockPriceFeed = address(4);
function setUp() public {
vm.prank(owner);
configBranch = new GlobalConfigurationBranch();
vm.mockCall(
mockERC20,
abi.encodeWithSelector(ERC20.decimals.selector),
abi.encode(18)
);
}
function testExploitZeroHeartbeat() public {
vm.prank(owner);
configBranch.configureMarginCollateral(
mockERC20,
1000000,
800000,
mockPriceFeed,
0
);
MarginCollateralConfiguration.Data memory config = configBranch.getMarginCollateralConfiguration(mockERC20);
assertEq(config.priceFeedHeartbeatSeconds, 0, "Heartbeat should be zero");
}
function testExploitLargeHeartbeat() public {
vm.prank(owner);
configBranch.configureMarginCollateral(
mockERC20,
1000000,
800000,
mockPriceFeed,
365 days
);
MarginCollateralConfiguration.Data memory config = configBranch.getMarginCollateralConfiguration(mockERC20);
assertEq(config.priceFeedHeartbeatSeconds, 365 days, "Heartbeat should be 365 days");
}
}
Impact
1: Acceptance of stale price data, causing incorrect collateral valuation
2: Manipulation of liquidation processes
3: Creation of arbitrage opportunities
4: System instability due to extreme heartbeat values
Tools Used
Manual review
Recommendations
1: Define MAX_HEARTBEAT_SECONDS as a constant with a reasonable upper limit
function configureMarginCollateral(
address collateralType,
uint128 depositCap,
uint120 loanToValue,
address priceFeed,
uint32 priceFeedHeartbeatSeconds
)
external
onlyOwner
{
if (priceFeedHeartbeatSeconds == 0 || priceFeedHeartbeatSeconds > MAX_HEARTBEAT_SECONDS) {
revert Errors.InvalidPriceFeedHeartbeat(priceFeedHeartbeatSeconds);
}
}
