INIT Capital Invitational
Findings & Analysis Report
2024-02-22
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] MarginTradingHook users could potentially be DOSed
- [M-02]
fillOrdernot properly cancel order when collateral of position is empty - [M-03] SwapType.CloseExactOut balance check too strict can be DOSed
- [M-04] LP unwrap / wrap is fully broken if master chef contract has insufficient reward token and block decollateralize wlp and wlp liquidation
-
Low Risk and Non-Critical Issues
- 01 Set treasury missing check
address(0) - 02 If the
order.recipientis blocklisted, his order can never be fulfilled - 03
fillOrdermay be subject to reentrancy - 04 Lack of view function for user to query if the
flashloancan be used - 05 Overpaid asset is locked in lending pool when flashloan
- 06 MarginTradeHook.sol#fillOrder does not validate order creator’s minHealth when filling the order
- 01 Set treasury missing check
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the INIT Capital smart contract system written in Solidity. The audit took place between January 26—February 2 2024.
Wardens
In Code4rena’s Invitational audits, the competition is limited to a small group of wardens; for this audit, 5 wardens contributed reports:
This audit was judged by hansfriese.
Final report assembled by PaperParachute.
Summary
The C4 analysis yielded an aggregated total of 7 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 4 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 4 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 2 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 INIT Capital repository, and is composed of 22 smart contracts written in the Solidity programming language and includes 2334 lines of Solidity code.
In addition to the known issues identified by the project team, an Automated Findings report was generated using the 4naly3er bot and all findings therein were classified as out of scope.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (3)
[H-01] MarginTradingHook#updateOrder lacks access control
Submitted by sashik_eth, also found by rvierdiiev and said
The MarginTradingHook#updateOrder function allows users to update their orders, it checks that the requested order is active order (L513) and that the function caller has opened position (L515). However, this function fails to check that user initPosId is equal to the initPosId saved in the order struct, meaning that the caller is an order creator:
File: MarginTradingHook.sol
503: function updateOrder(
504: uint _posId,
505: uint _orderId,
506: uint _triggerPrice_e36,
507: address _tokenOut,
508: uint _limitPrice_e36,
509: uint _collAmt
510: ) external {
511: _require(_collAmt != 0, Errors.ZERO_VALUE);
512: Order storage order = __orders[_orderId];
513: _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
514: uint initPosId = initPosIds[msg.sender][_posId];
515: _require(initPosId != 0, Errors.POSITION_NOT_FOUND);
516: MarginPos memory marginPos = __marginPositions[initPosId];
517: uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
518: _require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);
519:
520: order.triggerPrice_e36 = _triggerPrice_e36;
521: order.limitPrice_e36 = _limitPrice_e36;
522: order.collAmt = _collAmt;
523: order.tokenOut = _tokenOut;
524: emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
525: }Impact
Any order in the MarginTradingHook contract could be updated by other users.
Proof of Concept
The next test added to the TestMarginTradingHelper file could show a scenario when the user can update some other active orders:
function testUpdateNotOwnerOrder() public {
_setUpDexLiquidity(QUOTE_TOKEN, BASE_TOKEN);
address tokenIn = USDT;
address collToken = WETH;
address borrToken = USDT;
{
(uint posIdAlice, uint initPosIdAlice) = _openPos(tokenIn, collToken, borrToken, ALICE, 10_000, 10_000);
(uint posIdBob, ) = _openPos(tokenIn, collToken, borrToken, BOB, 10_000, 10_000);
uint markPrice_e36 = lens.getMarkPrice_e36(collToken, borrToken);
uint triggerPrice_e36 = markPrice_e36 * 9 / 10; // 90% from mark price
uint limitPrice_e36 = markPrice_e36 * 89 / 100; // 89% from mark price
address tokenOut = WETH;
MarginPos memory marginPos = hook.getMarginPos(initPosIdAlice);
uint orderIdAlice = _addStopLossOrder(
ALICE,
posIdAlice,
triggerPrice_e36,
tokenOut,
limitPrice_e36,
positionManager.getCollAmt(initPosIdAlice, marginPos.collPool)
);
vm.startPrank(BOB, BOB);
hook.updateOrder(posIdBob, orderIdAlice, triggerPrice_e36 - 1, tokenOut, limitPrice_e36, 1000);
vm.stopPrank();
Order memory order = hook.getOrder(orderIdAlice);
require(order.triggerPrice_e36 == triggerPrice_e36 - 1, 'order not updated');
}
}Recommended Mitigation Steps
Consider adding a check that prevents the possibility of updating arbitrary orders, similar to the cancelOrder function:
_require(order.initPosId == initPosId, Errors.INVALID_INPUT);[H-02] Order’s creator can update tokenOut to arbitrary token
Submitted by said, also found by ladboy233
When an order is created, it checks whether the order.tokenOut is equal to marginPos.baseAsset or _tokenOut is equal to marginPos.quoteAsset. However, when tokenOut is updated via updateOrder, it can be changed to an arbitrary token. This has the potential to be exploited by changing tokenOut to a high-value token right before the executor executes the fillOrder.
Proof of Concept
It can be observed that order’s creator can update order.tokenOut to arbitrary token.
function updateOrder(
uint _posId,
uint _orderId,
uint _triggerPrice_e36,
address _tokenOut,
uint _limitPrice_e36,
uint _collAmt
) external {
_require(_collAmt != 0, Errors.ZERO_VALUE);
Order storage order = __orders[_orderId];
_require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
uint initPosId = initPosIds[msg.sender][_posId];
_require(initPosId != 0, Errors.POSITION_NOT_FOUND);
MarginPos memory marginPos = __marginPositions[initPosId];
uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
_require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);
order.triggerPrice_e36 = _triggerPrice_e36;
order.limitPrice_e36 = _limitPrice_e36;
order.collAmt = _collAmt;
order.tokenOut = _tokenOut;
emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
}This can be exploited by the order’s creator. When the executor is about to execute the fillOrder, the order creator can front-run the operation by changing the order.tokenOut to a token with a high value. Consequently, the executor will transfer the amount that needs to be transferred but using this new high-value token. But this require hook already have approval from the executor to manage the new high value token.
Recommended Mitigation Steps
Validate the new token when updateOrder is called :
function updateOrder(
uint _posId,
uint _orderId,
uint _triggerPrice_e36,
address _tokenOut,
uint _limitPrice_e36,
uint _collAmt
) external {
_require(_collAmt != 0, Errors.ZERO_VALUE);
Order storage order = __orders[_orderId];
_require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
uint initPosId = initPosIds[msg.sender][_posId];
_require(initPosId != 0, Errors.POSITION_NOT_FOUND);
MarginPos memory marginPos = __marginPositions[initPosId];
uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
_require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);
+ _require(_tokenOut == marginPos.baseAsset || _tokenOut == marginPos.quoteAsset, Errors.INVALID_INPUT);
order.triggerPrice_e36 = _triggerPrice_e36;
order.limitPrice_e36 = _limitPrice_e36;
order.collAmt = _collAmt;
order.tokenOut = _tokenOut;
emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
}I think the severity should be high because it is very common the executor tries to fill a lot of order that have different token.
It is common that executor approves the hook to spend multiple tokens.
Order creator frontrun to update order to different token clearly violate order executor’s expectation if order creator update a low value token with high value token before the order is filled.
hansfriese (Judge) increased severity to High and commented:
I left a comment here.
[H-03] fillOrder executor can be front-run by the order creator by changing order’s limitPrice_e36, the executor’s assets can be stolen
Submitted by said, also found by rvierdiiev and ladboy233
limitPrice_e36 acts as slippage protection for the order creator, depending on the trade/order position (whether long or short). A higher or lower limit price impacts the tokenOut amount that needs to be transferred to the order’s creator. However, when the executor executes fillOrder, it can be front-run by the order creator to update limitPrice_e36 and steal tokens from the executor.
Proof of Concept
When fillOrder is executed, it will calculate the amtOut that needs to be transferred to order.recipient by calling _calculateFillOrderInfo.
function _calculateFillOrderInfo(Order memory _order, MarginPos memory _marginPos, address _collToken)
internal
returns (uint amtOut, uint repayShares, uint repayAmt)
{
(repayShares, repayAmt) = _calculateRepaySize(_order, _marginPos);
uint collTokenAmt = ILendingPool(_marginPos.collPool).toAmtCurrent(_order.collAmt);
// NOTE: all roundings favor the order owner (amtOut)
if (_collToken == _order.tokenOut) {
if (_marginPos.isLongBaseAsset) {
// long eth hold eth
// (2 * 1500 - 1500) = 1500 / 1500 = 1 eth
// ((c * limit - borrow) / limit
>>> amtOut = collTokenAmt - repayAmt * ONE_E36 / _order.limitPrice_e36;
} else {
// short eth hold usdc
// 2000 - 1 * 1500 = 500 usdc
// (c - borrow * limit)
>>> amtOut = collTokenAmt - (repayAmt * _order.limitPrice_e36 / ONE_E36);
}
} else {
if (_marginPos.isLongBaseAsset) {
// long eth hold usdc
// (2 * 1500 - 1500) = 1500 usdc
// ((c * limit - borrow)
>>> amtOut = (collTokenAmt * _order.limitPrice_e36).ceilDiv(ONE_E36) - repayAmt;
} else {
// short eth hold eth
// (3000 - 1 * 1500) / 1500 = 1 eth
// (c - borrow * limit) / limit
>>> amtOut = (collTokenAmt * ONE_E36).ceilDiv(_order.limitPrice_e36) - repayAmt;
}
}
}As can be observed, it heavily relies on limitPrice_e36 to calculate amtOut. A malicious order creator can front-run the execution of fillOrder and steal assets from the executor by changing limitPrice_e36, resulting in a high value of amtOut depending on the order’s position.
Recommended Mitigation Steps
Consider to add limitPrice_e36 check inside the fillOrder if it bigger than min/max provided limit price, revert the operation.
fez-init (INIT) confirmed and commented:
We will change the
updateOrderlogic to cancel and create a new order instead of modifying the existing order instead.
Medium Risk Findings (4)
[M-01] MarginTradingHook users could potentially be DOSed
Submitted by sashik_eth
The refundNative modifier unwraps and sends a native coin to the msg.sender at the end of the function execution. However, it could send native even if the case function call was not related to native coin trading - for example, if wrapped tokens were already on the MarginTradingHook balance. At the same time if the function caller is the contract that has no function except native transfer - the whole transaction would be reverted. Malicious actors could DOS such contracts by sending dust amounts of wrapped native tokens to the MarginTradingHook balance.
File: MarginTradingHook.sol
72: modifier refundNative() {
73: _;
74: // refund native token
75: uint wNativeBal = IERC20(WNATIVE).balanceOf(address(this));
76: // NOTE: no need receive function since we will use TransparentUpgradeableProxyReceiveETH
77: if (wNativeBal != 0) IWNative(WNATIVE).withdraw(wNativeBal);
78: uint nativeBal = address(this).balance;
79: if (nativeBal != 0) {
80: (bool success,) = payable(msg.sender).call{value: nativeBal}('');
81: _require(success, Errors.CALL_FAILED);
82: }
83: }Impact
Contracts that use MarginTradingHook could be DOSed in case they have no function to receive native.
Proof of Concept
Consider the next scenario:
- Alice creates a contract that she would use as the caller of
MarginTradingHook. Since Alice planning to work only with ERC20 tokens, she doesn’t implement thereceiveor payablefallbackfunction on her contract. - Alice using a contract in combination with
MarginTradingHookfor her trading activity for some time. - Bob sends to
MarginTradingHook1 wei of wrapped native token. - Now each call from Alice’s contract to
MarginTradingHookfunctions that include therefundNativemodifier would revert sinceMarginTradingHookunwraps 1 wei ofWNATIVEand tries to send it to the Alice contract. Since the last one can’t accept native transfers - calls revert.
Bob could effectively continue DOS even if native funds were withdrawn by other users, by spamming dust amount txs each block, at the same time Alice could lose funds on her active positions due to changing market conditions.
Recommended Mitigation Steps
Consider adding the explicit parameter refundNative that users could set to true in case they expect a native transfer. Alternatively, remove the require statement at the end of the refundNative modifier code.
fez-init (INIT) disputed and commented:
This can be QA. We will add a note for hook users to be able to receive native tokens. @hansfriese
hansfriese (Judge) decreased severity to Medium and commented:
Medium is appropriate because:
- There is no relevant note that hook users should be able to receive native tokens.
- Attackers might DOS others with 1 wei.
[M-02] fillOrder not properly cancel order when collateral of position is empty
Submitted by said, also found by sashik_eth
When fillOrder is triggered, and the order’s collateral size inside the position manager is empty, it should change the order status to canceled. However, due to mistakenly updating memory data instead of the storage __orders data, it will not be canceled properly.
Proof of Concept
It can be observed that when collateral size is empty, fillOrder will try to update order memory data status to OrderStatus.Cancelled and return the call.
function fillOrder(uint _orderId) external {
>>> Order memory order = __orders[_orderId];
_require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
MarginPos memory marginPos = __marginPositions[order.initPosId];
address collToken = ILendingPool(marginPos.collPool).underlyingToken();
address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
// if position is empty, cancel order
if (IPosManager(POS_MANAGER).getCollAmt(order.initPosId, marginPos.collPool) == 0) {
>>> order.status = OrderStatus.Cancelled;
emit CancelOrder(order.initPosId, _orderId);
return;
}
// validate trigger price condition
_validateTriggerPrice(order, marginPos);
// calculate fill order info
(uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
// transfer in repay tokens
IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
// transfer order owner's desired tokens to the specified recipient
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
// repay tokens
_ensureApprove(borrToken, repayAmt);
IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
// decollateralize coll pool tokens to executor
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
// update order status
__orders[_orderId].status = OrderStatus.Filled;
emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);
}However, this will not change the storage value of the order inside the __orders data. This will mislead users into assuming that the order is canceled when, in fact, it is not.
Recommended Mitigation Steps
Update __orders[_orderId].status storage data instead :
function fillOrder(uint _orderId) external {
Order memory order = __orders[_orderId];
_require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
MarginPos memory marginPos = __marginPositions[order.initPosId];
address collToken = ILendingPool(marginPos.collPool).underlyingToken();
address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
// if position is empty, cancel order
if (IPosManager(POS_MANAGER).getCollAmt(order.initPosId, marginPos.collPool) == 0) {
- order.status = OrderStatus.Cancelled;
+ __orders[_orderId].status = OrderStatus.Cancelled;
emit CancelOrder(order.initPosId, _orderId);
return;
}
// validate trigger price condition
_validateTriggerPrice(order, marginPos);
// calculate fill order info
(uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
// transfer in repay tokens
IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
// transfer order owner's desired tokens to the specified recipient
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
// repay tokens
_ensureApprove(borrToken, repayAmt);
IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
// decollateralize coll pool tokens to executor
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
// update order status
__orders[_orderId].status = OrderStatus.Filled;
emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);
}fez-init (INIT) confirmed, but disagreed with severity and commented:
This should be QA as there does not create impact.
I think Medium is appropriate as the order cancellation wouldn’t work as intended.
[M-03] SwapType.CloseExactOut balance check too strict can be DOSed
Submitted by ladboy233, also found by rvierdiiev and said
SwapType.CloseExactOut balance check too strict can be DOSed.
Proof of Concept
In CoreCallback function, we have the logic below:
uint amtOut = IERC20(swapInfo.tokenOut).balanceOf(address(this));
if (swapInfo.swapType == SwapType.OpenExactIn) {
// transfer to coll pool to mint
IERC20(swapInfo.tokenOut).safeTransfer(marginPos.collPool, amtOut);
emit SwapToIncreasePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtIn, amtOut);
} else {
// transfer to borr pool to repay
uint amtSwapped = amtIn;
if (swapInfo.swapType == SwapType.CloseExactOut) {
_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
amtSwapped -= IERC20(swapInfo.tokenIn).balanceOf(address(this));
}
emit SwapToReducePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtSwapped, amtOut);
}Note the logic below:
if (swapInfo.swapType == SwapType.CloseExactOut) {
_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
amtSwapped -= IERC20(swapInfo.tokenIn).balanceOf(address(this));
}This check is too strict:
_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);Malicious user can DOS the swap by simply perform a tiny swap by frontruning the swap to change the token ratio in the LP pool.
For example, the swapInfo.amtOut is 1000 USDC, and swapInfo.tokenOut is USDC.
It is difficult to get exactly 1000 USDC, so any swap that executes before the swap can make the received USDC amount to 1000.1 USDC or 999. USDC, then swap transaction reverts.
Recommended Mitigation Steps
Do not use ==, use >=
hansfriese (Judge) decreased severity to Medium and commented:
Downgrade to Medium as there is no direct fund loss.
[M-04] LP unwrap / wrap is fully broken if master chef contract has insufficient reward token and block decollateralize wlp and wlp liquidation
Submitted by ladboy233, also found by rvierdiiev
LP unwrap / wrap is fully broken if master chef contract has insufficient reward token.
Proof of Concept
We need to take a look at the external master chef contract that is not in the control of the init capital protocol team.
When deposit / withdraw / harvest / claim, this function _modify is called, which is this code
if (moeReward > 0) _moe.safeTransfer(account, moeReward);
if (address(extraRewarder) != address(0)) {
extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
}As we can see, when deposit / withdraw / harvest, the pending reward is transferred from master chef contract to msg.sender (which is the lp wrapper contract).
When calling extraRewarder.onModify, the reward is transferred from extra rewarder to wrapper contract.
But someone needs to transfer the moe token into the master chef to ensure there is sufficient reward token balance.
Someone needs to transfer the reward token into extraReward contract to ensure there is sufficient reward token balance.
In case when there are insufficient reward token in master chef contract and extraReward, the code will revert:
if (moeReward > 0) _moe.safeTransfer(account, moeReward);
if (address(extraRewarder) != address(0)) {
extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
}Suppose the reward accounting update is that the wlp contract is entitled to get 10000 moe token and 100 usdc token as extra reward, but in master chef there are only 9000 token, attempting to transfer the 10000 more tokens will revert.
The impact is severe because this revert would block lp unwrap and block original lp owner attemps to decollateralize wlp and make liquidation revert as well.
Recommended Mitigation Steps
When regular withdraw failed, the code should call emergencyWithdraw.
This function does not claim reward, but at least this function can ensure withdraw wlp when decollateralize lp or liquidation transaction does not revert.
Low Risk and Non-Critical Issues
For this audit, 4 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by ladboy233 received the top score from the judge.
The following wardens also submitted reports: 0x73696d616f, sashik_eth, and rvierdiiev.
| Number | Title |
|---|---|
| 1 | Set treasury missing check address(0) |
| 2 | Lack of function way to collateralize and decollateralize using WLP in MarginTradingHook.sol |
| 3 | WLPMoeMasterChef reward token list can be outdated when master chef update extra reward contract |
| 4 | If the order.recipient is blocklisted, his order can never be fulfilled |
| 5 | Order cannot be filed in certain case and lack of view function to know if the order can be fulfilled |
| 6 | FillOrder may subject to reentrancy |
| 7 | Flashloan does not charge fee |
| 8 | Lack of view function for user to query if the flashloan can be used |
[01] Set treasury missing check address(0)
The code should validate treasury address is not address(0) when setting Treasury in [lending pool](https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/lendingpool/LendingPool.sol#L243).
function setTreasury(address _treasury) external accrue onlyGovernor {
treasury = _treasury;
emit SetTreasury(_treasury);
}Tn fact by default the treasury address is not set, and accrue interest would revert when minting interest to treasury address(0).
function _mint(address account, uint256 value) internal {
if (account == address(0)) {
revert ERC20InvalidReceiver(address(0));
}
_update(address(0), account, value);
}The reward token is never removed, but the problem is that the master chef admin can remove extra reward or update extra reward.
Suppose the reward token list has [MOE, and WETH], WETH is the extra reward, then the extra reward changes to [JOE], then WETH token data is considered outdated.
[02] If the order.recipient is blocklisted, his order can never be fulfilled
When creating a order in margin trading hook, the code record order.recipient
Token support blocklisting behavior:
https://github.com/d-xo/weird-erc20?tab=readme-ov-file#tokens-with-blocklists
But when filling the order, if the order.recipient is blocklisted, the order can never be fulfilled and fill order always revert.
[03] fillOrder may be subject to reentrancy
The function fillOrder set order status as filled after external token transfer.
https://docs.openzeppelin.com/contracts/3.x/erc777
This opens room for reentrancy if the token is ERC777 token when external user can control the execution flow via the receive hook.
The external token transfer is this line of code:
// transfer order owner's desired tokens to the specified recipient
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);Where order.recipient can re-enter the fillOrder to fill order more than one times it is recommended to add nonReentrant modifer to the function fillOrder.
[04] Lack of view function for user to query if the flashloan can be used
It is recommended to implement and adopt [https://eips.ethereum.org/EIPS/eip-3156] where there are views function for external user to call to query if the flashloan can be used:
function maxFlashLoan(
address token
) external view returns (uint256);
/**
* @dev The fee to be charged for a given loan.
* @param token The loan currency.
* @param amount The amount of tokens lent.
* @return The amount of `token` to be charged for the loan, on top of the returned principal.
*/
function flashFee(
address token,
uint256 amount
) external view returns (uint256);[05] Overpaid asset is locked in lending pool when flashloan
Note: At the judge’s request here, this downgraded issue from the same warden has been included in this report for completeness.
Overpaid asset in locked when flashloan
Proof of Concept
When flashloan, in this end to code check if the lending pool balance is greater than the consumed balance before flashloan:
// execute callback
IFlashReceiver(msg.sender).flashCallback(_pools, _amts, _data);
// check pool balance after callback
for (uint i; i < _pools.length; i = i.uinc()) {
_require(IERC20(tokens[i]).balanceOf(_pools[i]) >= balanceBefores[i], Errors.INVALID_AMOUNT_TO_REPAY);
}The problem is that in case when user over-pay flashloan.
Suppose user flash loan 10000 USDC and then repay 10100 USDC, the 100 USDC is lost and locked in the lending pool because the internal accounting of lending pool use cashAmount to track available balance.
However, the code above does not update cash amount after flashloan is finished.
Recommended Mitigation Steps
while I understand this is an mitigation for finding
https://github.com/code-423n4/2023-12-initcapital-findings/issues/3
the issue above can be resolved (issue 3)
// check that flash is enabled
_require(poolConfig.canFlash, Errors.FLASH_PAUSED);When setModeStatus.canCollateralize and can.Borrow to false
/// @inheritdoc IConfig
function setModeStatus(uint16 _mode, ModeStatus calldata _status) external onlyGuardian {
_require(_mode != 0, Errors.INVALID_MODE);
__modeConfigs[_mode].status = _status;
emit SetModeStatus(_mode, _status);
}Disable all flashloan for all the pool that belongs to a mode and sync the lending pool balance even user overpays flashloan
[06] MarginTradeHook.sol#fillOrder does not validate order creator’s minHealth when filling the order
Note: At the judge’s request here, this downgraded issue from the same warden has been included in this report for completeness.
MarginTradeHook.sol#fillOrder does not validate order creator’s minHealth when filling the order
Proof of Concept
In MarginTradingHook.sol, user can create stopLoss or take profits order, this is the logic for fulfilling order:
IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
// transfer order owner's desired tokens to the specified recipient
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
// repay tokens
_ensureApprove(borrToken, repayAmt);
IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
// decollateralize coll pool tokens to executor
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
// update order status
__orders[_orderId].status = OrderStatus.Filled;As we can see, first, the borr token is transfered in to prepare for repayment then the msg.sender pays order.recipient with token amount out:
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);Then, the we repay for order creator:
_ensureApprove(borrToken, repayAmt);
IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);Then the msg.sender that fulfill the order can remove order creator’s collateral:
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);The problem is that when removing order creator’s collateral amount, unlike create Margin Position where the code give user’s option to validate the account min_health, the fillOrder logic cannot let order creator specify minaccounthealth after order fulfillment.
Let us consider the case below:
- user create a margin position with 1 WEHT as collateral
- user create a order and set order.coll amount to 0.5 WETH, this is ok for user, because even when the order is fulfilled, 0.5 WETH goes to order fulfiller, but there are 0.5 WETH collateral position left to ensure account health
- user adjust margin position and remove 0.4 WETH collateral, but the order info remain unchanged, there is 0.6 WETH left as collateral
- the user’s order is fulfilled, but there is only 0.1 WETH left as collateral,
- WETH price drops, user get liquidated because the fulfilled order remove the majority of collateral
This can be avoided if user can specify the account min health.
If user say, after order fulfilled, the account health cannot be below a certain threshold.
Fulfilling order that remove too much collateral can revert and user will not subject to liquidation.
The root cause is also this order.collAmount can become stale.
After user removeCollateral, the order.collAmount can become stale.
Recommended Mitigation Steps
Allow the user to specify min account health when creating the order:
function addStopLossOrder(
uint _posId,
uint _triggerPrice_e36,
address _tokenOut,
uint _limitPrice_e36,
uint _collAmt,
uint min_account_health // here
) external returns (uint orderId) {
orderId = _createOrder(_posId, _triggerPrice_e36, _tokenOut, _limitPrice_e36, _collAmt, min_account_health, OrderType.StopLoss);
}Validate the min_health when they fulfill the order:
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
// added code here
_validateHealth(order.initPosId, uint order.min_health)
// update order status
__orders[_orderId].status = OrderStatus.Filled;
emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);Gas Optimizations
For this audit, 2 reports were submitted by wardens detailing gas optimizations. The report highlighted below by rvierdiiev received the top score from the judge.
The following wardens also submitted reports: ladboy233.
[G-01] Move some event from InitCore to PosManager.
To decrease bytecode size of InitCore contract you can move some events emitting to PosManager.
https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L165 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L217 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L230 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L245 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L267 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/core/InitCore.sol#L285
emit CreatePosition(msg.sender, posId, _mode, _viewer);
emit SetPositionMode(_posId, _mode);
emit Collateralize(_posId, _pool, amtColl);
emit Decollateralize(_posId, _pool, _to, amtDecoll);
emit CollateralizeWLp(_posId, _wLp, _tokenId, amtColl);
emit DecollateralizeWLp(_posId, _wLp, _tokenId, _to, amtDecoll);[G-02] Remove check inside InitCore.callback function.
The check inside InitCore.callback function is not needed, because InitCore contract even don’t have coreCallback function. You can decrease bytecode size by removing the check.
[G-03] Move mode check to PosManager.
To decrease size of InitCore contract, you can move 0 mode check to the PosManager.
[G-04] Remove arrays check.
InitCore._validateFlash function does arrays check. This line can be removed and there will be no problems with that even if sizes will be different.
Audit Analysis
For this audit, 2 analysis reports were submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by ladboy233 received the top score from the judge.
The following wardens also submitted reports: rvierdiiev.
For WLpMoeMasterChef.sol
Overview
The WLpMoeMasterChef.sol contract is designed to allow users to wrap their Liquidity Provider (LP) tokens into wrapped LP (WLP) tokens. These WLP tokens can then be used as collateral for borrowing funds, integrating with a broader decentralized finance (DeFi) ecosystem.
Concerns and Recommendations
-
External MasterChef Integration:
- The contract relies on external MasterChef contracts for reward distributions. It’s critical to ensure these external contracts are reliable and secure.
- There should be mechanisms to handle scenarios where the MasterChef contract doesn’t have sufficient rewards, to prevent disruptions in the WLP wrapping and unwrapping processes.
-
Handling of External Calls:
- External calls, especially those related to reward distributions, should be designed to not block critical functionalities like LP wrapping or unwrapping.
- Implementing fail-safe mechanisms or timeouts for external calls could mitigate risks associated with these dependencies.
-
Risk of Blocking Withdrawals/Liquidations:
- Any failure in the external calls or issues in the reward mechanism should not hinder users’ ability to withdraw or liquidate their positions. This is crucial for maintaining trust and functionality in crisis scenarios.
For MarginTradingBook.sol
Overview
The MarginTradingBook.sol contract seems to handle margin trading functionalities, including the creation and management of orders such as stop-loss or take-profit.
Concerns and Recommendations
-
Race Conditions in Repay and Liquidation:
- The possibility of race conditions between repay actions and liquidations needs to be carefully managed. Synchronized checks or locking mechanisms might be necessary.
-
Front-Running Risk in Order Creation and Filling:
- The system should be safeguarded against the possibility of order creators front-running order fills to manipulate outcomes. This could be addressed through mechanisms like time locks or penalties for abnormal behaviors.
-
Uncertainty in Order Fulfillment:
- Orders may fail to be filled for various reasons, such as outdated collateral amounts, blocklisted recipients, or the liquidation of the user’s collateral.
- Implementing a function to preview if an order can be fulfilled would enhance transparency and usability. This function should account for all possible reasons an order might fail.
-
Recommendation for Order Management:
- It’s advisable to have a mechanism to cancel unfillable orders automatically or provide users with the ability to do so manually. This would prevent cluttering the system with stagnant orders and improve efficiency.
-
Overall System Robustness:
- The interplay between different components (like MarginTradingBook, external contracts, and the broader ecosystem) needs to be tested thoroughly under various scenarios, especially edge cases, to ensure the resilience and reliability of the system.
Time spent:
15 hours
Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.