A malicious owner can observe and frontrun calls to InfinityStaker#rageQuit to increase penalties and extract the quitter's full staked balance as a penalty. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock.
Scenario:
Alice stakes her tokens for 12 months
Alice later decides to ragequit and withdraw her staked tokens, and calls rageQuit
The contract owner observes Alice's transaction and frontruns it with a call to updatePenalties, increasing the 12-month penalty to 1
Alice's full balance is deducted as a penalty.
Malicious owner can frontrun trades
A malicious owner can observe and frontrun trades to increase the protocol fee and extract the full trade amount as a fee. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock. Additionally, consider adding and enforcing an upper bound on the protocol fee parameter.
Scenario:
Alice calls takeOrders to fill an open order for her token
The contract owner observes Alice's transaction and frontruns it with a call to setProtocolFee, increasing the fee amount to 100%
The full amount of Alice's trade is extracted as a protocol fee
Noncritical
Emit events from protected functions
Protected functions in InfinityStaker update staking parameters like the treasury address, ragequit penalties, and level thresholds, but do not emit corresponding events. Consider adding events that log updates to these parameters. This enables end users to monitor changes to these parameters, and the protocol to monitor for potential malicious behavior.
Note that Solidity versions 0.8.13 and 0.8.14 are vulnerable to a recently reported optimizer bug related to inline assembly. This project's current settings use the via-IR optimizer pipeline, which is not affected, and there is limited usage of assembly in this codebase and its dependencies, but it's worth being aware of this issue. Solidity 0.8.15 has been released with a fix.
/// @dev Gets current order price for orders that vary in price over time (dutch and reverse dutch auctions)
function _getCurrentPrice(OrderTypes.MakerOrder calldata order) internal view returns (uint256) {
(uint256 startPrice, uint256 endPrice) = (order.constraints[1], order.constraints[2]);
uint256 duration = order.constraints[4] - order.constraints[3];
uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
if (priceDiff == 0 || duration == 0) {
return startPrice;
}
uint256 elapsedTime = block.timestamp - order.constraints[3];
uint256 PRECISION = 10**4; // precision for division; similar to bps
uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
priceDiff = (priceDiff * portionBps) / PRECISION;
return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;
}
Consider extracting this shared function to a library: this removes duplication and ensures the two separate functions will not diverge and behave differently.
Missing event indexes
Event indexes simplify filtering for events by specific criteria, like user addresses.
Low
Malicious owner can frontrun
rageQuitA malicious owner can observe and frontrun calls to
InfinityStaker#rageQuitto increase penalties and extract the quitter's full staked balance as a penalty. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock.Scenario:
rageQuitupdatePenalties, increasing the 12-month penalty to1Malicious owner can frontrun trades
A malicious owner can observe and frontrun trades to increase the protocol fee and extract the full trade amount as a fee. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock. Additionally, consider adding and enforcing an upper bound on the protocol fee parameter.
Scenario:
takeOrdersto fill an open order for her tokensetProtocolFee, increasing the fee amount to 100%Noncritical
Emit events from protected functions
Protected functions in
InfinityStakerupdate staking parameters like the treasury address, ragequit penalties, and level thresholds, but do not emit corresponding events. Consider adding events that log updates to these parameters. This enables end users to monitor changes to these parameters, and the protocol to monitor for potential malicious behavior.InfinityStaker#updateStakeLevelThresholdInfinityStaker#updatePenaltiesInfinityStaker#updateInfinityTreasuryOptimizer bug in Solidity 0.8.13 and 0.8.14
Note that Solidity versions 0.8.13 and 0.8.14 are vulnerable to a recently reported optimizer bug related to inline assembly. This project's current settings use the via-IR optimizer pipeline, which is not affected, and there is limited usage of assembly in this codebase and its dependencies, but it's worth being aware of this issue. Solidity 0.8.15 has been released with a fix.
QA
Duplication of
_getCurrentPricefunctionAn identical
_getCurrentPricefunction is included in bothInfinityOrderBookComplicationandInfinityExchange:Consider extracting this shared function to a library: this removes duplication and ensures the two separate functions will not diverge and behave differently.
Missing event indexes
Event indexes simplify filtering for events by specific criteria, like user addresses.
Consider adding event indexes for:
address userinCancelAllOrdersaddress userinCancelMultipleOrdersaddress sellerandaddress buyerinMatchOrderFulfilledaddress sellerandaddress buyerinTakeOrderFulfilledTypos
StorageinInfinityExchange#L77ordersinOrderTypes#L39unstakeinInfinityStaker#L112