Platform: Code4rena
Start Date: 16/10/2023
Pot Size: $60,500 USDC
Total HM: 16
Participants: 131
Period: 10 days
Judge: 0xTheC0der
Total Solo HM: 3
Id: 296
League: ETH
Rank: 8/131
Findings: 7
Award: $671.15
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xpiken
Also found by: 0xCiphky, 0xComfyCat, 0xStalin, 0xhegel, 0xkazim, 3docSec, AM, Aymen0909, CaeraDenoir, DeFiHackLabs, Drynooo, Eigenvectors, Fulum, HALITUS, HChang26, Jiamin, Juntao, LokiThe5th, Mike_Bello90, MiloTruck, QiuhaoLi, Silvermist, SovaSlava, SpicyMeatball, T1MOH, Toshii, TrungOre, TuringConsulting, Vagner, Yanchuan, ZdravkoHr, _nd_koo, almurhasan, audityourcontracts, ayden, cartlex_, circlelooper, crunch, cu5t0mpeo, deth, erictee, ggg_ttt_hhh, gizzy, gumgumzum, hash, jasonxiale, josephdara, ke1caM, kodyvim, lanrebayode77, marqymarq10, max10afternoon, nirlin, nonseodion, osmanozdemir1, peter, radev_sw, rvierdiiev, said, serial-coder, sl1, smiling_heretic, squeaky_cactus, stackachu, tallo, trachev, zaevlad
0.0606 USDC - $0.06
According to the gitbook a borrower can close one of his markets at any time, given that the market has enough funds available to pay for all scheduled withdrawals and all fees accrued. This functionality is implemented in the WildcatMarket.closeMarket():
function closeMarket() external onlyController nonReentrant { MarketState memory state = _getUpdatedState(); state.annualInterestBips = 0; state.isClosed = true; state.reserveRatioBips = 0; if (_withdrawalData.unpaidBatches.length() > 0) { revert CloseMarketWithUnpaidWithdrawals(); } uint256 currentlyHeld = totalAssets(); uint256 totalDebts = state.totalDebts(); if (currentlyHeld < totalDebts) { // Transfer remaining debts from borrower asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld); } else if (currentlyHeld > totalDebts) { // Transfer excess assets to borrower asset.safeTransfer(borrower, currentlyHeld - totalDebts); } _writeState(state); emit MarketClosed(block.timestamp); }
The above function is protected by the onlyController modifier, making it possible only for the controller to call it.
modifier onlyController() { if (msg.sender != controller) revert NotController(); _; }
The WildcatMarketController, however, does not call this function ever. This makes closing markets impossible.
Run the following test in WildcatMarketController.t.sol:
function testFailCloseMarket() public { MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18); vm.startPrank(parameters.borrower); asset.approve(address(controller), 20e18); MarketControllerParameters memory marketParameters = controllerFactory .getMarketControllerParameters(); WildcatMarket newMarket = WildcatMarket( controller.deployMarket( address(mockDAI), "Market1", "M1", type(uint128).max, 2500, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, 1000, 3 days ) ); newMarket.closeMarket(); }
Foundry
Protect the closeMarket function with onlyBorrower instead of onlyController:
- function closeMarket() external onlyController nonReentrant { + function closeMarket() external onlyBorrower nonReentrant { ... }
Access Control
#0 - c4-pre-sort
2023-10-27T07:18:18Z
minhquanym marked the issue as duplicate of #147
#1 - c4-judge
2023-11-07T13:53:20Z
MarioPoneder changed the severity to 2 (Med Risk)
#2 - c4-judge
2023-11-07T14:04:59Z
MarioPoneder marked the issue as partial-50
#3 - c4-judge
2023-11-07T14:16:53Z
MarioPoneder changed the severity to 3 (High Risk)
🌟 Selected for report: 0xpiken
Also found by: 0xCiphky, 0xComfyCat, 0xStalin, 0xhegel, 0xkazim, 3docSec, AM, Aymen0909, CaeraDenoir, DeFiHackLabs, Drynooo, Eigenvectors, Fulum, HALITUS, HChang26, Jiamin, Juntao, LokiThe5th, Mike_Bello90, MiloTruck, QiuhaoLi, Silvermist, SovaSlava, SpicyMeatball, T1MOH, Toshii, TrungOre, TuringConsulting, Vagner, Yanchuan, ZdravkoHr, _nd_koo, almurhasan, audityourcontracts, ayden, cartlex_, circlelooper, crunch, cu5t0mpeo, deth, erictee, ggg_ttt_hhh, gizzy, gumgumzum, hash, jasonxiale, josephdara, ke1caM, kodyvim, lanrebayode77, marqymarq10, max10afternoon, nirlin, nonseodion, osmanozdemir1, peter, radev_sw, rvierdiiev, said, serial-coder, sl1, smiling_heretic, squeaky_cactus, stackachu, tallo, trachev, zaevlad
0.0606 USDC - $0.06
According to the gitbook a borrower can change the max total capacity of his markets. This functionality is implemented in the WildcatMarketConfig.setMaxTotalSupply():
function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant { MarketState memory state = _getUpdatedState(); if (_maxTotalSupply < state.totalSupply()) { revert NewMaxSupplyTooLow(); } state.maxTotalSupply = _maxTotalSupply.toUint128(); _writeState(state); emit MaxTotalSupplyUpdated(_maxTotalSupply); }
The above function is protected by the onlyController modifier, making it possible only for the controller to call it.
modifier onlyController() { if (msg.sender != controller) revert NotController(); _; }
The WildcatMarketController, however, does not call this function ever. This makes changing capacities impossible.
Run the following test in WildcatMarketController.t.sol:
function testFailCapacityChange() public { MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18); vm.startPrank(parameters.borrower); asset.approve(address(controller), 20e18); MarketControllerParameters memory marketParameters = controllerFactory .getMarketControllerParameters(); WildcatMarket newMarket = WildcatMarket( controller.deployMarket( address(mockDAI), "Market1", "M1", type(uint128).max, 2500, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, 1000, 3 days ) ); newMarket.setMaxTotalSupply(newMarket.maxTotalSupply() + 1); }
Foundry
Protect the setMaxTotalSupply function with onlyBorrower instead of onlyController:
- function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant { + function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyBorrower nonReentrant { ... }
Access Control
#0 - c4-pre-sort
2023-10-27T06:22:12Z
minhquanym marked the issue as duplicate of #162
#1 - c4-pre-sort
2023-10-27T06:58:24Z
minhquanym marked the issue as duplicate of #147
#2 - c4-judge
2023-11-07T13:52:15Z
MarioPoneder marked the issue as partial-50
#3 - c4-judge
2023-11-07T14:16:53Z
MarioPoneder changed the severity to 3 (High Risk)
🌟 Selected for report: 0xStalin
Also found by: 0xCiphky, 0xComfyCat, 0xbepresent, 3docSec, Eigenvectors, Fulum, HChang26, Infect3d, QiuhaoLi, SandNallani, SovaSlava, TrungOre, YusSecurity, ZdravkoHr, ast3ros, ayden, bdmcbri, cu5t0mpeo, elprofesor, gizzy, jasonxiale, kodyvim, marqymarq10, max10afternoon, nisedo, nobody2018, radev_sw, rvierdiiev, serial-coder, xeros
6.5602 USDC - $6.56
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L121
In order for lenders to interact with a given market, they have to be authorized in the given market's controller. There are functions for authorizing and deauthorizing lenders. An _authorizedLenders EnumerableSets exists to keep track of authorized lenders.
The market uses different roles to give appropriate permissions to the users having them. One of them is the WithdrawOnly role. It is meant to be given to a deauthorized lender to stop him from future deposits. This is achieved by calling updateLenderAuthorization.
updateLenderAuthorization() calls WildcatMarketConfig.updateAccountAuthorization() with the lender and a flag as arguments. The flag is true if the lender is present in the set and false otherwise.
WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
WildcatMarketConfig.updateAccountAuthorization() then sets the account's role to AuthRole.DepositAndWithdraw if the flag is true, otherwise - to AuthRole.WithdrawOnly
if (_isAuthorized) { account.approval = AuthRole.DepositAndWithdraw; } else { account.approval = AuthRole.WithdrawOnly; }
This is problematic because updateLenderAuthorization() can be called for any account by any account. This means that if it is called for an account that was never authorized in first place, that account will get the WithdrawOnly role and will be able to withdraw.
Run the following test in WildcatMarketController.t.sol
function testUnauthorizedLender() public { MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18); address lender1 = address(0x1ed1); address unauthorizedLender = address(0x6afce2); uint256 initialAmount = 20e18; vm.startPrank(parameters.borrower); asset.approve(address(controller), initialAmount); MarketControllerParameters memory marketParameters = controllerFactory .getMarketControllerParameters(); WildcatMarket newMarket = WildcatMarket( controller.deployMarket( address(mockDAI), "Market1", "M1", type(uint128).max, 0, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, 1000, 3 days ) ); address[] memory lenders = new address[](2); lenders[0] = lender1; controller.authorizeLenders(lenders); vm.stopPrank(); uint256 lenderBalance = 10000e18; deal(address(mockDAI), lender1, lenderBalance); vm.startPrank(lender1); mockDAI.approve(address(newMarket), lenderBalance); newMarket.depositUpTo(lenderBalance); newMarket.transfer(unauthorizedLender, newMarket.balanceOf(lender1)); vm.stopPrank(); address[] memory markets = new address[](1); markets[0] = address(newMarket); vm.prank(unauthorizedLender); controller.updateLenderAuthorization(unauthorizedLender, markets); vm.prank(unauthorizedLender); newMarket.queueWithdrawal(9000e18); vm.warp( marketParameters.minimumWithdrawalBatchDuration + block.timestamp ); newMarket.executeWithdrawal( unauthorizedLender, uint32(block.timestamp) ); assertEq(mockDAI.balanceOf(unauthorizedLender), 9000e18); }
Foundry
My first thought was making updateLenderAuthorization internal and calling it each time a lender is authorized/deauthorized. However, this will make the deauthorization of blocked accounts impossible, since updateLenderAuthorization reverts if it accesses a blocked account.
Another possibility seemed like using low-level call to invoke the updateLenderAuthorization to avoid reverting. This is also not a solution. updateLenderAuthorization has to be public because otherwise unblocked lenders will not be able to get back their WithdrawOnly role in order to withraw their funds.
Therefore, my recommendation is to add another enumerable set that tracks the deauthorized lenders and in the updateLenderAuthorization call updateAccountAuthorization with:
Invalid Validation
#0 - c4-pre-sort
2023-10-27T08:40:21Z
minhquanym marked the issue as duplicate of #400
#1 - c4-pre-sort
2023-10-27T08:51:22Z
minhquanym marked the issue as duplicate of #155
#2 - c4-judge
2023-11-07T12:54:53Z
MarioPoneder marked the issue as satisfactory
#3 - c4-judge
2023-11-07T12:59:29Z
MarioPoneder marked the issue as selected for report
#4 - laurenceday
2023-11-09T09:44:59Z
Replica of https://github.com/code-423n4/2023-10-wildcat-findings/issues/266, a finding that has already been selected for the report as a High Risk. Mitigation in there.
#5 - MarioPoneder
2023-11-09T17:38:20Z
This finding and its duplicates do not discuss the impact of bypassing sanctions by abusing the vulnerability.
In contrast to #266 and its duplicates, therefore I agree with the pre-sort about two distinct findings having different severities.
Was selected for report due to PoC test case and overall discussion of the issue.
#6 - MiloTruck
2023-11-13T14:26:35Z
Hi @MarioPoneder, since this finding has identified the same root cause as #266, it should be a duplicate.
The proper way to handle a scenario where two issues have the same root cause, but one has failed to identify the highest severity impact is to duplicate all of them under one finding and award them with partial credit.
This is mentioned in the C4 documentation:
All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path.
However, any submissions which do not identify or effectively rationalize the top identified severity case may be judged as “partial credit” and may have their shares in that finding’s pie divided by 2 or 4 at judge’s sole discretion (e.g. 50% or 25% of the shares of a satisfactory submission in the duplicate set).
This has also been mentioned in the recent supreme court standardization:
Similar exploits under a single issue The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.
Given the above, when similar exploits would demonstrate different impacts, the highest, most irreversible would be the one used for scoring the finding. Duplicates of the finding will be graded based on the achieved impact relative to the Submission Chosen for Report.
Reason being that having two distinct findings would essentially be rewarding the same bug twice in award calculation, making it unfair towards other findings.
This finding also has less duplicates than #266, so it could end up getting a higher payout even though it did not identify the highest severity impact.
#7 - MarioPoneder
2023-11-14T13:57:50Z
Thank you for the valuable input.
We already have 2 primary findings in this contents where some duplicates were awarded with partial credit.
In this case, the assessed impacts differ to an extent that even led to different severities of both primary findings, which seemed to justify their separation.
However, considering the fairness of the resulting reward distribution and the supreme court standardization, it's best to move forward with duplication and partial credit.
#8 - c4-judge
2023-11-14T13:59:03Z
MarioPoneder changed the severity to 3 (High Risk)
#9 - c4-judge
2023-11-14T14:02:43Z
MarioPoneder marked the issue as duplicate of #266
#10 - c4-judge
2023-11-14T14:03:14Z
MarioPoneder marked the issue as not selected for report
#11 - c4-judge
2023-11-14T14:03:22Z
MarioPoneder marked the issue as partial-50
🌟 Selected for report: YusSecurity
Also found by: 0xAsen, 0xCiphky, 0xDING99YA, 0xKbl, 0xSwahili, 0xbepresent, 3docSec, AS, Aymen0909, DeFiHackLabs, GREY-HAWK-REACH, KeyKiril, MiloTruck, QiuhaoLi, Silvermist, SovaSlava, TrungOre, VAD37, Vagner, Yanchuan, ZdravkoHr, ast3ros, cartlex_, d3e4, deth, ggg_ttt_hhh, gizzy, kodyvim, nirlin, nobody2018, rvierdiiev, serial-coder, sl1, tallo, xeros
6.6715 USDC - $6.67
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L169 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L172-L175 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L98
The protocol supports blacklisting of users. When a lender enters the blacklist, an escrow that holds his assets until whitelisting is created. Currently, there are 2 places in the code that create a new escrow - WildcatMarketBase._blockAccount() and WildcatMarketWithdrawals.executeWithdrawal(). The createEscrow function looks like this:
function createEscrow(address borrower, address account, address asset)
However, both _blockAccount and executeWithdrawal call it like this:
createEscrow(accountAddress, borrower, address(asset))
When a new escrow is created the borrower will be incorrectly set to the account and the account - to the borrower. Therefor, eligible lenders will never be able to get their funds back from the escrow.
Manual Review
Fix the escrow creations:
- createEscrow(accountAddress, borrower, address(asset)) + createEscrow(borrower, accountAddress, address(asset))
Error
#0 - c4-pre-sort
2023-10-27T02:24:55Z
minhquanym marked the issue as duplicate of #515
#1 - c4-judge
2023-11-07T11:50:06Z
MarioPoneder marked the issue as satisfactory
🌟 Selected for report: MiloTruck
Also found by: 0xCiphky, LokiThe5th, Madalad, Robert, ZdravkoHr, nonseodion
218.5317 USDC - $218.53
https://github.com/code-423n4/2023-10-wildcat/blob/18bda5daf6a668cfe5dd6c0749a908e193e7b90c/src/libraries/LibStoredInitCode.sol#L112-L115 https://github.com/code-423n4/2023-10-wildcat/blob/18bda5daf6a668cfe5dd6c0749a908e193e7b90c/src/libraries/LibStoredInitCode.sol#L92-L95
The LibStoreInitCode.sol library is used to deploy intermediate contracts that hold others' contracts initialization code in their runtime code. This feature is used in deployMarket:
LibStoredInitCode.create2WithStoredInitCode(marketInitCodeStorage, salt);
The issue is that the create2WithStoredInitCode function does not check if the creation of the new contract is successful.
assembly { let initCodePointer := mload(0x40) let initCodeSize := sub(extcodesize(initCodeStorage), 1) extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize) deployment := create2(value, initCodePointer, initCodeSize, salt) }
This means that if the deployment fails, the supposed "market" will still be registered and even worse, the borrower will have to pay the protocol fee:
originationFeeAsset.safeTransferFrom(borrower, parameters.feeRecipient, originationFeeAmount);
A deployment can fail if:
if ((parameters.protocolFeeBips > 0).and(parameters.feeRecipient == address(0))) { revert FeeSetWithoutRecipient(); } if (parameters.annualInterestBips > BIP) { revert InterestRateTooHigh(); } if (parameters.reserveRatioBips > BIP) { revert ReserveRatioBipsTooHigh(); } if (parameters.protocolFeeBips > BIP) { revert InterestFeeTooHigh(); } if (parameters.delinquencyFeeBips > BIP) { revert PenaltyFeeTooHigh(); }
A PoC with foundry using the third revert case. Add it in WildcatMarketController.t.sol
function testUnsuccessfulMarketDeployment() public { uint256 initialAmount = 20e18; uint80 feeAmount = 1e18; address invalidToken = address(0xaf3141); deal(address(asset), parameters.borrower, initialAmount); vm.prank(archController.owner()); controllerFactory.setProtocolFeeConfiguration(address(0xfee), address(asset), feeAmount, 1000); uint256 balanceBefore = asset.balanceOf(parameters.borrower); vm.startPrank(parameters.borrower); asset.approve(address(controller), initialAmount); MarketControllerParameters memory marketParameters = controllerFactory.getMarketControllerParameters(); address newMarket = controller.deployMarket(invalidToken, "Market1", "M1", 4000e18, 0, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, marketParameters.minimumReserveRatioBips, marketParameters.minimumDelinquencyGracePeriod); uint256 codeSize; assembly { codeSize := extcodesize(newMarket) } assertEq(codeSize, 0); assertEq(asset.balanceOf(parameters.borrower), balanceBefore - feeAmount); vm.stopPrank(); }
Foundry
After deployment, check its success:
if eq(extcodesize(deployment), 0) { revert(0, 0) }
Token-Transfer
#0 - c4-pre-sort
2023-10-27T04:39:27Z
minhquanym marked the issue as primary issue
#1 - c4-pre-sort
2023-10-27T04:45:18Z
minhquanym marked the issue as sufficient quality report
#2 - c4-sponsor
2023-10-31T21:35:40Z
laurenceday (sponsor) confirmed
#3 - c4-judge
2023-11-07T14:50:00Z
MarioPoneder marked the issue as satisfactory
#4 - c4-judge
2023-11-07T15:02:43Z
MarioPoneder marked issue #499 as primary and marked this issue as a duplicate of 499
🌟 Selected for report: 3docSec
Also found by: 0xCiphky, 0xbepresent, 0xbrett8571, Eigenvectors, MiloTruck, Toshii, Tricko, TrungOre, ZdravkoHr, b0g0, caventa, cu5t0mpeo, deth, ggg_ttt_hhh, gizzy, joaovwfreire, josephdara, serial-coder, smiling_heretic, stackachu
16.6643 USDC - $16.66
Upon market creation, the borrower specifies annual interest BIP that has to be in the range of MinimumAnnualInterestBips and MaximumAnnualInterestBips. After that the borrower is free to change the annual interest to whatever value he wants to using WildcatMarketController.setAnnualInterestBips():
function setAnnualInterestBips( address market, uint16 annualInterestBips ) external virtual onlyBorrower onlyControlledMarket(market) { // If borrower is reducing the interest rate, increase the reserve // ratio for the next two weeks. if (annualInterestBips < WildcatMarket(market).annualInterestBips()) { TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[market]; if (tmp.expiry == 0) { tmp.reserveRatioBips = uint128(WildcatMarket(market).reserveRatioBips()); // Require 90% liquidity coverage for the next 2 weeks WildcatMarket(market).setReserveRatioBips(9000); } tmp.expiry = uint128(block.timestamp + 2 weeks); } WildcatMarket(market).setAnnualInterestBips(annualInterestBips); }
The function does not check if the provided value is between the minimum and maximum range. This allows the borrower to sets whatever interest he wants to. While there is a mechanism that will be fired when a borrower decreases the interest APR - the reserve ratio will be set to 90% for 2 weeks - I still believe this issue is of medium severity because it violates one of the main invariants stated on the contest's page:
Market parameters should never be able to exit the bounds defined by the controller which deployed it.
An example when things may go wrong is if a borrower makes a mistake and sets the interest to a really high value, causing the scaleFactor to increase very fast and in result not being able to pay for that market anymore.
Change the MinimumAnnualInterestBips to a value bellow 1000 (because there are other deployed test markets with an interest rate of 1000 or more) and run the following test in WildcatMarketController.t.sol:
function testInterestChange() public { MockERC20 mockDAI = new MockERC20("MOCKDAI", "mDAI", 18); vm.startPrank(parameters.borrower); asset.approve(address(controller), 20e18); MarketControllerParameters memory marketParameters = controllerFactory .getMarketControllerParameters(); WildcatMarket newMarket = WildcatMarket( controller.deployMarket( address(mockDAI), "Market1", "M1", type(uint128).max, 2500, marketParameters.minimumDelinquencyFeeBips, marketParameters.minimumWithdrawalBatchDuration, 1000, 3 days ) ); uint16 minBIPS = controller .getParameterConstraints() .minimumAnnualInterestBips; controller.setAnnualInterestBips( address(newMarket), minBIPS - minBIPS / 2 ); }
Foundry
function setAnnualInterestBips( address market, uint16 annualInterestBips ) external virtual onlyBorrower onlyControlledMarket(market) { // If borrower is reducing the interest rate, increase the reserve // ratio for the next two weeks. + assertValueInRange( + annualInterestBips, + MinimumAnnualInterestBips, + MaximumAnnualInterestBips, + AnnualInterestBipsOutOfBounds.selector + ); if (annualInterestBips < WildcatMarket(market).annualInterestBips()) { TemporaryReserveRatio storage tmp = temporaryExcessReserveRatio[ market ]; if (tmp.expiry == 0) { tmp.reserveRatioBips = uint128( WildcatMarket(market).reserveRatioBips() ); // Require 90% liquidity coverage for the next 2 weeks WildcatMarket(market).setReserveRatioBips(9000); } tmp.expiry = uint128(block.timestamp + 2 weeks); } WildcatMarket(market).setAnnualInterestBips(annualInterestBips); }
Invalid Validation
#0 - c4-pre-sort
2023-10-27T14:20:39Z
minhquanym marked the issue as duplicate of #443
#1 - c4-judge
2023-11-07T12:27:00Z
MarioPoneder marked the issue as satisfactory
🌟 Selected for report: J4X
Also found by: 0x3b, 0xCiphky, 0xComfyCat, 0xStalin, 0xbepresent, 3docSec, DavidGiladi, DeFiHackLabs, Drynooo, Fulum, GREY-HAWK-REACH, InAllHonesty, MatricksDeCoder, Mike_Bello90, MiloTruck, Phantom, SHA_256, T1MOH, Udsen, VAD37, YusSecurity, ZdravkoHr, ast3ros, caventa, deepkin, deth, devival, ggg_ttt_hhh, inzinko, jasonxiale, josieg_74497, karanctf, ke1caM, nisedo, nobody2018, nonseodion, osmanozdemir1, radev_sw, rvierdiiev, serial-coder, t0x1c
10.1663 USDC - $10.17
Currently, the way that borrowers must repay funds to the markets is by directly transferring funds to them. After that they have to call updateState. This 2-step process adds additional overhead. This is a problem if the borrower initiates 2 different transactions because the second may be stuck in the mempool, or the borrower may just forget to call updateState. If a borrower repays until the reserve ratio is met, but updateState() is not called and the market is not used for a long time, the period from the repayment to the next market interaction will be wrongly claimed to be delinquency period.
Manual Review, Foundry
Add a function which allows the borrower to repay his assets and after that calls updateState
Timing
#0 - c4-pre-sort
2023-10-28T11:41:42Z
minhquanym marked the issue as duplicate of #76
#1 - c4-judge
2023-11-08T16:37:04Z
MarioPoneder changed the severity to QA (Quality Assurance)
#2 - c4-judge
2023-11-09T15:09:56Z
MarioPoneder marked the issue as grade-b
🌟 Selected for report: rahul
Also found by: 0xSmartContract, InAllHonesty, J4X, Sathish9098, ZdravkoHr, catellatech, hunter_w3b, invitedtea, nirlin, radev_sw
412.5049 USDC - $412.50
Description | Impact | Likelihood |
---|---|---|
Wrong argument ordering in createEscrow | HIGH | HIGH |
Markets can never be closed | HIGH | HIGH |
Description | Impact | Likelihood |
---|---|---|
CREATE2 success is not checked | MEDIUM | LOW |
Everyone can get the WithdrawOnly role | MEDIUM | HIGH |
Borrower can set market interest outside of the minimimum and maximum range | MEDIUM | HIGH |
Borrower may have to pay more fee for delinquency than needed | HIGH | LOW |
Max total supply cannot be changed | MEDIUM | HIGH |
Description | Severity |
---|---|
Lender's deposit may be stuck in the mempool | LOW |
Market token name missing space | NC |
Escrow address is not stored in WildcatMarket | NC |
Mistake in a comment | NC |
Add an early return |
---|
During this 10-days audit the codebase was tested in various different ways. The first step after cloning the repository was ensuring that all tests provided pass successfully when run. After that began the phase when I got some high-level overview of the functionality by reading the code. There were some parts of the project that caught my attention. These parts were tested with Foundry POCs. Some of them were false positives, others turned out to be true positives that were reported. The contest's main page does a wonderful job providing main invariants that should never be broken and ideas for exploits. The gitbook is also very good resource, even though some inconsistencies and wrong numbers were found there. These were fixed by the sponsors. I also had a look at the external contracts that the protocol integrates with, for example, OpenZeppelin's EnumerableSet.
Wildcat is a protocol that lets borrowers take uncollateralized loans from approved lenders. It inverts the typical model of lending and borrowing in DEFI - borrowers create vaults and lenders deposit their funds in these vaults. Via reserve ratio percents, lenders' liquidity plays the role of their own collateral. In order for a borrower to be eligible to deploy a market he has to first sign a Service Agreement offchain.
The "grandparent" of the protocol is the ArchController contract. It handles registration of borrowers, controllers and factories. A borrower that is registered in the ArchController can then use a registered factory to deploy a controller. The factory holds the settings for the controller that when deployed, makes a call to it to fetch them.
All deployments in the protocol happen through the LibStoredInitCode library. Let's have a look at an example of deploying a controller. The idea is that we deploy a contract which holds the controller's creation code prefixed by a STOP opcode (0x00) to avoid executing the code as a smart contract:
Byte | Opcode | Stack |
---|---|---|
0x61 | PUSH1 | 0x00ad |
0x5f | PUSH0 | 0x00 |
The next opcode is 0x39 (codecopy). It will copy a part of the init code and will store it in memory. It will store it in 0x00, will start copying from 0x0a (10) and will copy 0x00ad bytes. Lets take a look at the beginning of the init code again: 0x6100ad5f81600a5f39f300. Starting from 0x0a, this will copy the code from the end of the prefix code (which is 0x00, or the STOP) to the end of the controller's creation code. The code is now copied in memory and we have the following stack:
Byte | Opcode | Stack |
---|---|---|
0x61 | PUSH1 | 0x00ad |
0x5f | PUSH0 | 0x00 |
The next and final opcode is 0xf3 (return). We return 0x0ad bytes loading from 0x00. Which is the controller's initialization code prepended with a stop opcode. This is now the runtime code of the newly created contract.
When a borrower creates a controller, he then authorizes the lenders that he wants to interact with. He also deploys a market using this controller. Each market works with only one ERC20 token. A fee for creating a new market may be enabled.
The authorized lenders can deposit their ERC20 funds in a market they are approved for by calling either deposit or depositUpTo and specified amount.
Lenders can also withdraw funds that they have deposited. First, they have to queue a new withdrawal:
The main idea of the protocol is to let borrowers borrow. This happens when a eligible account calls the borrow function:
There are several interest and taxes mechanisms in the protocol:
The whole interest system is cleverly managed by using a scaleFactor in RAYs.
When a lender deposits funds, the amount deposited (called normal amount) is divided by that scaleFactor and the result is recorded as a scaledAmount. As time passes and interest accrues, the scaleFactor is increased. Then, when converting back to a normal amount, the scaledAmount is multiplied by the scaleFactor. This means that as more time passes the same scaledAmount will be equal to a larger and larger normal amount.
This codebase's quality is good even though some vulnerabilities were found. It makes use of clever techniques for deploying contracts and internal balance tracking. The code's tests have a good coverage too. However, they are somehow convoluted and hard to be understand at first.
During my analysis of the codebase, I thought some changes may be done to improve the protocol:
There are some centralization risks. For example, if the account of the archcontroller's owner gets compromised, a lot of things may go wrong. Protocol fee may be set to a too high one, and the fee recipient to a foreign entity. This will cause a huge lose of money.
There also exists the risk of a borrower losing his private key. The developers have thought of that scenario letting other accounts pay borrower's debt on his behalf by just transferring tokens. This may be used by the same borrower that lost his private keys.
50 hours
#0 - c4-pre-sort
2023-10-29T15:04:01Z
minhquanym marked the issue as high quality report
#1 - laurenceday
2023-11-06T10:03:07Z
Enjoyed this one!
#2 - c4-sponsor
2023-11-06T10:03:12Z
laurenceday (sponsor) acknowledged
#3 - c4-judge
2023-11-09T12:23:28Z
MarioPoneder marked the issue as grade-b
#4 - c4-judge
2023-11-09T17:15:35Z
MarioPoneder marked the issue as grade-a