Ondo Finance contest
Findings & Analysis Report
2023-02-28
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] Admin should be able to refund or redeem the sanctioned users
- [M-02] First Deposit Bug
- [M-03]
CashManager.setEpochDurationfunctions has inconsistent output - [M-04] KYCRegistry is susceptible to signature replay attack.
- [M-05]
setPendingRedemptionBalance()may cause the user’s cash token to be lost
-
Low Risk and Non-Critical Issues
- Summary
- L-01 Token transfer to address(0) should be avoided
- L-02 Wrong if statement in the function
setMintFee - L-03 Giving KYC status to address(0) should be forbidden
- L-04 Redeem limit shouldn’t be set below the
currentMintAmount - L-05
completeRedemptionsis vulnerable to admin mistakes - N-01 Mandatory checks for extra safety
- N-02 Constructor lacks address(0) check
- N-03 Missing check to ensure epoch duration isn’t set as zero
- N-04 Upgradeable contract is missing a
__gap[50]storage variable - N-05 Create your own import names instead of using the regular ones
- N-06 Initialize function does not use the initializer modifier
- N-07 Use delete to clear variables instead of zero assignment
- N-08 Confusing function comment on
setMinimumDepositAmount - N-09 Unnecessary if statement in
_checkAndUpdateRedeemLimit - N-10 Pause/Unpause functions should emit event to notify users
- N-11 Two KYC checks made on the same redeemers
- N-12 Unused constructor
- N-13 NatSpec is incomplete in the pausing functions
- N-14 Unnecessary KYC check on the payer
- R-01 Shorthand way to write if / else statement
- R-02 Modifier should be used instead of require on admin functions
- R-03 Unused variables should be deleted
- R-04 Use require instead of assert
- R-05 Immutable should be used on variables that can’t be changed
- R-06 Unnecessary return statement applied
- R-07 Numeric values having to do with time should use time units for readability
- O-01 Floating pragma
- O-02 Outdated Compiler Version
- O-03 Function Naming suggestions
- O-04 Proper use of get as a function name prefix
- O-05 Events is missing indexed fields
-
- Overview
- G-01 Using immutable on variables that are only set in the constructor and never after (2.1k gas per var)
- G-02 Tightly pack storage variables/optimize the order of variable declaration (Gas Savings: 6k in total)
- G-03 Massive 15k per tx gas savings - use 1 and 2 for Reentrancy guard
- G-04 The result of a function call should be cached rather than re-calling the function
- G-05 Cache the mapping values rather than fetch it every time
- G-06 Internal/Private functions only called once can be inlined to save gas
- G-07 Multiple accesses of a mapping/array should use a local variable cache
- G-08 Emitting storage values instead of the memory one.
- G-09 Using storage instead of memory for structs/arrays saves gas
- G-10 Refactor the code here to avoid storage readings
- G-11 Duplicated require()/revert() checks should be refactored to a modifier or function
- G-12 x += y costs more gas than x = x + y for state variables
- G-13 Using unchecked blocks to save gas
- G-14 Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)
- G-15 Splitting require() statements that use && saves gas - (saves 8 gas per &&)
- G-16 Reorder the require statements to have the less gas consuming before the expensive one
- G-17 Caching global variables is more expensive than using the actual variable(use msg.sender instead of caching it)
- G-18 Use a more recent version of solidity
- 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 contest 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 contest outlined in this document, C4 conducted an analysis of the Ondo Finance smart contract system written in Solidity. The audit contest took place between January 11—January 17 2023.
Wardens
74 Wardens contributed reports to the Ondo Finance contest:
- 0x1f8b
- 0x52
- 0x5rings
- 0xAgro
- 0xSmartContract
- 0xcm
- 0xjuicer
- 0xkato
- 2997ms
- AkshaySrivastav
- Aymen0909
- BClabs (nalus and Reptilia)
- BPZ (pa6221, Bitcoinfever244 and PrasadLak)
- BRONZEDISC
- Bauer
- Bnke0x0
- CodingNameKiki
- Deekshith99
- Diana
- IllIllI
- Josiah
- Kaysoft
- RaymondFam
- Rolezn
- SaeedAlipoor01988
- Sathish9098
- SleepingBugs (Deivitto and 0xLovesleep)
- Tajobin
- Udsen
- Viktor_Cortess
- adriro
- arialblack14
- betweenETHlines
- bin2chen
- btk
- c3phas
- cccz
- chaduke
- chrisdior4
- cryptostellar5
- cryptphi
- csanuragjain
- cygaar
- defsec
- descharre
- dharma09
- erictee
- eyexploit
- gzeon
- halden
- hansfriese
- horsefacts
- immeas
- joestakey
- koxuan
- lukris02
- luxartvinsec
- minhquanym
- nicobevi
- oyc_109
- pavankv
- peanuts
- rbserver
- saneryee
- scokaf (Scoon and jauvany)
- shark
- tnevler
- tsvetanovv
- zaskoh
This contest was judged by Trust.
Final report assembled by itsmetechjay and liveactionllama.
Summary
The C4 analysis yielded an aggregated total of 6 unique vulnerabilities. Of these vulnerabilities, 1 received a risk rating in the category of HIGH severity and 5 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 54 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 24 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 Ondo Finance contest repository, and is composed of 19 smart contracts, 5 abstracts, and 6 interfaces written in the Solidity programming language and includes 4,365 lines of Solidity code.
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 (1)
[H-01] Loss of user funds when completing CASH redemptions
Submitted by adriro, also found by minhquanym, minhquanym, zaskoh, cccz, and peanuts
The function completeRedemptions present in the CashManager contract is used by the manager to complete redemptions requested by users and also to process refunds.
https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L707-L727
function completeRedemptions(
address[] calldata redeemers,
address[] calldata refundees,
uint256 collateralAmountToDist,
uint256 epochToService,
uint256 fees
) external override updateEpoch onlyRole(MANAGER_ADMIN) {
_checkAddressesKYC(redeemers);
_checkAddressesKYC(refundees);
if (epochToService >= currentEpoch) {
revert MustServicePastEpoch();
}
// Calculate the total quantity of shares tokens burned w/n an epoch
uint256 refundedAmt = _processRefund(refundees, epochToService);
uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
.totalBurned - refundedAmt;
uint256 amountToDist = collateralAmountToDist - fees;
_processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
collateral.safeTransferFrom(assetSender, feeRecipient, fees);
emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
}The total refunded amount that is returned from the internal call to _processRefund is then used to calculate the effective amount of CASH burned (redemptionInfoPerEpoch[epochToService].totalBurned - refundedAmt). This resulting value is then used to calculate how much each user should receive based on how much CASH they redeemed and the total amount that was burned.
The main issue here is that the refunded amount is not updated in the totalBurned storage variable for the given epoch. Any subsequent call to this function won’t take into account refunds from previous calls.
Impact
If the manager completes the refunds and redemptions at different steps or stages for a given epoch, using multiple calls to the completeRedemptions, then any refunded amount won’t be considered in subsequent calls to the function.
Any redemption that is serviced in a call after a refund will be calculated using the total burned without subtracting the previous refunds. The function completeRedemptions will call the internal function _processRedemption passing the burned amount as the quantityBurned argument, the value is calculated in line 755:
https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L755
uint256 collateralAmountDue = (amountToDist * cashAmountReturned) /
quantityBurned;This means that redemptions that are processed after one or more previous refunds will receive less collateral tokens even if they redeemed the same amount of CASH tokens (i.e. greater quantityBurned, less collateralAmountDue), causing loss of funds for the users.
Proof of Concept
In the following test, Alice, Bob and Charlie request a redemption. The admin first calls completeRedemptions to process Alice’s request and refund Charlie. The admin then makes a second call to completeRedemptions to process Bob’s request. Even though they redeemed the same amount of CASH (each 200e18), Alice gets 150e6 tokens while Bob is sent ~133e6.
contract TestAudit is BasicDeployment {
function setUp() public {
createDeploymentCash();
// Grant Setter
vm.startPrank(managerAdmin);
cashManager.grantRole(cashManager.SETTER_ADMIN(), address(this));
cashManager.grantRole(cashManager.SETTER_ADMIN(), managerAdmin);
vm.stopPrank();
// Seed address with 1000000 USDC
vm.prank(USDC_WHALE);
USDC.transfer(address(this), INIT_BALANCE_USDC);
}
function test_CashManager_completeRedemptions_BadReedem() public {
_setupKYCStatus();
// Seed alice and bob with 200 cash tokens
_seed(200e18, 200e18, 50e18);
// Have alice request to withdraw 200 cash tokens
vm.startPrank(alice);
tokenProxied.approve(address(cashManager), 200e18);
cashManager.requestRedemption(200e18);
vm.stopPrank();
// Have bob request to withdraw 200 cash tokens
vm.startPrank(bob);
tokenProxied.approve(address(cashManager), 200e18);
cashManager.requestRedemption(200e18);
vm.stopPrank();
// Have charlie request to withdraw his tokens
vm.startPrank(charlie);
tokenProxied.approve(address(cashManager), 50e18);
cashManager.requestRedemption(50e18);
vm.stopPrank();
// Move forward to the next epoch
vm.warp(block.timestamp + 1 days);
vm.prank(managerAdmin);
cashManager.setMintExchangeRate(2e6, 0);
// Approve the cashMinter contract from the assetSender account
_seedSenderWithCollateral(300e6);
// First call, withdraw Alice and refund Charlie
address[] memory withdrawFirstCall = new address[](1);
withdrawFirstCall[0] = alice;
address[] memory refundFirstCall = new address[](1);
refundFirstCall[0] = charlie;
vm.prank(managerAdmin);
cashManager.completeRedemptions(
withdrawFirstCall, // Addresses to issue collateral to
refundFirstCall, // Addresses to refund cash
300e6, // Total amount of money to dist incl fees
0, // Epoch we wish to process
0 // Fee amount to be transferred to ondo
);
// Alice redemption is calculated taking the refund into account
uint256 aliceExpectedBalance = 200e18 * 300e6 / ((200e18 + 200e18 + 50e18) - 50e18);
assertEq(USDC.balanceOf(alice), aliceExpectedBalance);
assertEq(USDC.balanceOf(bob), 0);
assertEq(tokenProxied.balanceOf(charlie), 50e18);
// Second call, withdraw Bob
address[] memory withdrawSecondCall = new address[](1);
withdrawSecondCall[0] = bob;
address[] memory refundSecondCall = new address[](0);
vm.prank(managerAdmin);
cashManager.completeRedemptions(
withdrawSecondCall, // Addresses to issue collateral to
refundSecondCall, // Addresses to refund cash
300e6, // Total amount of money to dist incl fees
0, // Epoch we wish to process
0 // Fee amount to be transferred to ondo
);
// But here, Bob's redemption doesn't consider the previous refund.
uint256 bobBadBalance = uint256(200e18 * 300e6) / (200e18 + 200e18 + 50e18);
assertEq(USDC.balanceOf(bob), bobBadBalance);
}
function _setupKYCStatus() internal {
// Add KYC addresses
address[] memory addressesToKYC = new address[](5);
addressesToKYC[0] = guardian;
addressesToKYC[1] = address(cashManager);
addressesToKYC[2] = alice;
addressesToKYC[3] = bob;
addressesToKYC[4] = charlie;
registry.addKYCAddresses(kycRequirementGroup, addressesToKYC);
}
function _seed(
uint256 aliceAmt,
uint256 bobAmt,
uint256 charlieAmt
) internal {
vm.startPrank(guardian);
tokenProxied.mint(alice, aliceAmt);
tokenProxied.mint(bob, bobAmt);
tokenProxied.mint(charlie, charlieAmt);
vm.stopPrank();
}
function _seedSenderWithCollateral(uint256 usdcAmount) internal {
vm.prank(USDC_WHALE);
USDC.transfer(assetSender, usdcAmount);
vm.prank(assetSender);
USDC.approve(address(cashManager), usdcAmount);
}
}Recommended Mitigation Steps
Update the totalBurned amount to consider refunds resulting from the call to _processRefund:
function completeRedemptions(
address[] calldata redeemers,
address[] calldata refundees,
uint256 collateralAmountToDist,
uint256 epochToService,
uint256 fees
) external override updateEpoch onlyRole(MANAGER_ADMIN) {
_checkAddressesKYC(redeemers);
_checkAddressesKYC(refundees);
if (epochToService >= currentEpoch) {
revert MustServicePastEpoch();
}
// Calculate the total quantity of shares tokens burned w/n an epoch
uint256 refundedAmt = _processRefund(refundees, epochToService);
uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
.totalBurned - refundedAmt;
+ redemptionInfoPerEpoch[epochToService].totalBurned = quantityBurned;
uint256 amountToDist = collateralAmountToDist - fees;
_processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
collateral.safeTransferFrom(assetSender, feeRecipient, fees);
emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
}ali2251 (Ondo Finance) confirmed
ypatil12 (Ondo Finance) resolved
Medium Risk Findings (5)
[M-01] Admin should be able to refund or redeem the sanctioned users
Submitted by hansfriese
Sanctioned user’s funds are locked.
Proof of Concept
It is understood that the sanctioned users can not mint nor redeem because the functions requestMint() and requestRedemption() are protected by the modifier checkKYC().
And it is also understood that the protocol team knows about this.
But I still believe the admin should be able to refund or redeem those funds.
And it is not possible for now because the KYC is checked for the redeemers and refundees in the function completeRedemptions().
So as long as the user becomes unverified (due to several reasons including the signature expiry), the funds are completely locked and even the admin has no control over it.
CashManager.sol
707: function completeRedemptions(
708: address[] calldata redeemers,
709: address[] calldata refundees,
710: uint256 collateralAmountToDist,
711: uint256 epochToService,
712: uint256 fees
713: ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
714: _checkAddressesKYC(redeemers);
715: _checkAddressesKYC(refundees);
716: if (epochToService >= currentEpoch) {
717: revert MustServicePastEpoch();
718: }
719: // Calculate the total quantity of shares tokens burned w/n an epoch
720: uint256 refundedAmt = _processRefund(refundees, epochToService);
721: uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
722: .totalBurned - refundedAmt;
723: uint256 amountToDist = collateralAmountToDist - fees;
724: _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
725: collateral.safeTransferFrom(assetSender, feeRecipient, fees);
726: emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
727: }Recommended Mitigation Steps
Assuming that the MANAGER_ADMIN can be trusted, I suggest removing KYC check for the redeemers and refundees.
ali2251 (Ondo Finance) disputed and commented:
It’s not in scope as mentioned in README, specifically in Not in scope ->
KYC/Sanction related edge cases specifically when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols or being liquidated in Flux
I don’t believe this clause includes the described case, i.e. even admin cannot move the locked funds.
[M-02] First Deposit Bug
Submitted by AkshaySrivastav
The CToken is a yield bearing asset which is minted when any user deposits some units of
underlying tokens. The amount of CTokens minted to a user is calculated based upon
the amount of underlying tokens user is depositing.
As per the implementation of CToken contract, there exists two cases for CToken amount calculation:
- First deposit - when
CToken.totalSupply()is0. - All subsequent deposits.
Here is the actual CToken code (extra code and comments clipped for better reading):
function exchangeRateStoredInternal() internal view virtual returns (uint) {
uint _totalSupply = totalSupply;
if (_totalSupply == 0) {
return initialExchangeRateMantissa;
} else {
uint totalCash = getCashPrior();
uint cashPlusBorrowsMinusReserves = totalCash +
totalBorrows -
totalReserves;
uint exchangeRate = (cashPlusBorrowsMinusReserves * expScale) /
_totalSupply;
return exchangeRate;
}
}
function mintFresh(address minter, uint mintAmount) internal {
// ...
Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});
uint actualMintAmount = doTransferIn(minter, mintAmount);
uint mintTokens = div_(actualMintAmount, exchangeRate);
totalSupply = totalSupply + mintTokens;
accountTokens[minter] = accountTokens[minter] + mintTokens;
}The Bug
The above implementation contains a critical bug which can be exploited to steal funds of initial depositors of a freshly deployed CToken contract.
As the exchange rate is dependent upon the ratio of CToken’s totalSupply and underlying token balance of CToken contract, the attacker can craft transactions to manipulate the exchange rate.
Steps to attack:
- Once the CToken has been deployed and added to the lending protocol, the attacker mints the smallest possible amount of CTokens.
-
Then the attacker does a plain
underlyingtoken transfer to the CToken contract, artificially inflating theunderlying.balanceOf(CToken)value.Due to the above steps, during the next legitimate user deposit, the
mintTokensvalue for the user will become less than1and essentially be rounded down to0by Solidity. Hence the user gets0CTokens against his deposit and the CToken’s entire supply is held by the Attacker. - The Attacker can then simply
redeemhis CToken balance for the entireunderlyingtoken balance of the CToken contract.
The same steps can be performed again to steal the next user’s deposit.
It should be noted that the attack can happen in two ways:
- The attacker can simply execute Step 1 and 2 as soon as the CToken gets added to the lending protocol.
- The attacker watches the pending transactions of the network and frontruns the user’s deposit transaction by executing Step 1 and 2 and then backruns it with Step 3.
Impact
A sophisticated attack can impact all user deposits until the lending protocols owners and users are notified and contracts are paused. Since this attack is a replicable attack, it can be performed continuously to steal the deposits of all depositors that try to deposit into the CToken contract.
The loss amount will be the sum of all deposits done by users into the CToken multiplied by the underlying token’s price.
Suppose there are 10 users and each of them tries to deposit 1,000,000 underlying tokens into the CToken contract. Price of underlying token is $1.
Total loss (in $) = $10,000,000
Proof of Concept
New test case was added to forge-tests/lending/fToken/fDAI.t.sol
function test_bug_firstMintIssue() public {
address attacker = alice;
seedUserDAI(attacker, 2_000_000e18);
seedUserDAI(bob, 1_000_000e18);
assertEq(fDAI.exchangeRateStored(), 2e26);
assertEq(fDAI.totalSupply(), 0);
assertEq(fDAI.balanceOf(attacker), 0);
vm.prank(attacker);
DAI.approve(address(fDAI), type(uint256).max);
vm.prank(attacker);
fDAI.mint(2e8);
assertEq(fDAI.balanceOf(attacker), 1);
assertEq(fDAI.totalSupply(), 1);
vm.prank(bob);
DAI.approve(address(fDAI), type(uint256).max);
// Front-running
vm.prank(attacker);
DAI.transfer(address(fDAI), 1_000_000e18);
assertEq(fDAI.getCash(), 1_000_000e18 + 2e8);
vm.prank(bob);
fDAI.mint(1_000_000e18);
assertEq(fDAI.balanceOf(bob), 0);
assertEq(fDAI.totalSupply(), 1);
vm.prank(attacker);
fDAI.redeem(1);
assertEq(DAI.balanceOf(attacker), 3_000_000e18);
assertEq(fDAI.totalSupply(), 0);
}The Fix
The fix to prevent this issue would be to enforce a minimum deposit that cannot be withdrawn. This can be done by minting a small amount of CToken units to 0x00 address on the first deposit.
function mintFresh(address minter, uint mintAmount) internal {
// ...
Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});
uint actualMintAmount = doTransferIn(minter, mintAmount);
uint mintTokens = div_(actualMintAmount, exchangeRate);
/// THE FIX
if (totalSupply == 0) {
totalSupply = 1000;
accountTokens[address(0)] = 1000;
mintTokens -= 1000;
}
totalSupply = totalSupply + mintTokens;
accountTokens[minter] = accountTokens[minter] + mintTokens;
// ...
}Instead of a fixed 1000 value an admin controlled parameterized value can also be used to control the burn amount on a per CToken basis.
ali2251 (Ondo Finance) confirmed
ypatil12 (Ondo Finance) commented:
This is a bug, we get around this operationally by minting fTokens and burning when initializing the market. See our proposal here.
Leaving as Medium severity as likelihood is low but potential impact is high + sponsor found it valuable.
[M-03] CashManager.setEpochDuration functions has inconsistent output
Submitted by AkshaySrivastav, also found by bin2chen
The CashManager contract contains setEpochDuration function which is used by MANAGER_ADMIN role to update the epochDuration parameter.
function setEpochDuration(uint256 _epochDuration) external onlyRole(MANAGER_ADMIN) {
uint256 oldEpochDuration = epochDuration;
epochDuration = _epochDuration;
emit EpochDurationSet(oldEpochDuration, _epochDuration);
}The result of the setEpochDuration function execution can be impacted any external agent. The epochDuration is a crucial parameter of CashManager contract which determines the length of epochs in the contract.
The issue here is that the setEpochDuration function updates the epochDuration value without invoking the transitionEpoch function first.
This leads to two different end results and scenarios:
- When
transitionEpochis executed beforesetEpochDurationby an external agent (front-running). - When
transitionEpochis executed aftersetEpochDurationby an external agent (back-running).
In these two different cases, the duration and epoch number of last few passed epochs can be impacted differently. The result becomes dependent upon the wish of the external agent.
The exact impact is demonstrated in the PoC below.
Proof of Concept
New test cases were added to forge-tests/cash/cash_manager/Setters.t.sol file.
function test_bug_inconsistentOutputOf_setEpochDuration_Case1() public {
// skip 1 epoch duration
vm.warp(block.timestamp + 1 days);
// here the setEpochDuration() txn is frontrunned by issuing the transitionEpoch() txn
cashManager.transitionEpoch();
// this is the setEpochDuration() txn which was frontrunned
cashManager.setEpochDuration(2 days);
assertEq(cashManager.currentEpoch(), 1);
vm.warp(block.timestamp + 2 days);
cashManager.transitionEpoch();
assertEq(cashManager.currentEpoch(), 2); // number of epochs after 3 days is 2
}
function test_bug_inconsistentOutputOf_setEpochDuration_Case2() public {
// skip 1 epoch duration
vm.warp(block.timestamp + 1 days);
// here we wait for the setEpochDuration() to be validated on the network
cashManager.setEpochDuration(2 days);
// then we backrun the setEpochDuration() txn with transitionEpoch() txn
cashManager.transitionEpoch();
assertEq(cashManager.currentEpoch(), 0);
vm.warp(block.timestamp + 2 days);
cashManager.transitionEpoch();
assertEq(cashManager.currentEpoch(), 1); // number of epochs after 3 days is 1
}Recommended Mitigation Steps
The transitionEpoch function should be executed before executing the setEpochDuration function so that the values for passed epochs are recorded in a consistent way. This can be done by adding the updateEpoch modifier.
function setEpochDuration(uint256 _epochDuration) external updateEpoch onlyRole(MANAGER_ADMIN) {
uint256 oldEpochDuration = epochDuration;
epochDuration = _epochDuration;
emit EpochDurationSet(oldEpochDuration, _epochDuration);
}ali2251 (Ondo Finance) confirmed
ypatil12 (Ondo Finance) resolved
[M-04] KYCRegistry is susceptible to signature replay attack.
Submitted by AkshaySrivastav, also found by adriro, csanuragjain, Tajobin, rbserver, gzeon, immeas, Bauer, and 0xjuicer
The KYCRegistry contract uses signatures to grant KYC status to the users using the addKYCAddressViaSignature function.
However this function does not prevent replaying of signatures in the case where KYC status was revoked from a user.
function addKYCAddressViaSignature( ... ) external {
require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
require(
!kycState[kycRequirementGroup][user],
"KYCRegistry: user already verified"
);
require(block.timestamp <= deadline, "KYCRegistry: signature expired");
bytes32 structHash = keccak256(
abi.encode(_APPROVAL_TYPEHASH, kycRequirementGroup, user, deadline)
);
bytes32 expectedMessage = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(expectedMessage, v, r, s);
_checkRole(kycGroupRoles[kycRequirementGroup], signer);
kycState[kycRequirementGroup][user] = true;
// ...
}This function could be exploited in the case when these conditions are true:
- KYC status was granted to user using a signature with validity up to
deadline. - Before the
deadlinewas passed, the KYC status of user was revoked using theremoveKYCAddressesfunction.
In the abovementioned conditions, the malicious user can submit the original signature again to the addKYCAddressViaSignature function which will forcefully grant the KYC status to the malicious user again.
It should also be noted that due to this bug until the deadline has passed, the privileged accounts cannot revoke the KYC status of a KYC granted user. This can result in unwanted moving of funds by the user in/out of Ondo protocol.
Proof of Concept
Test file created BugTest.t.sol and was run by forge test --mp ./forge-tests/BugTest1.t.sol
pragma solidity 0.8.16;
import "forge-std/Test.sol";
import "forge-std/Vm.sol";
import "contracts/cash/kyc/KYCRegistry.sol";
contract SanctionsList {
function isSanctioned(address) external pure returns (bool) {
return false;
}
}
struct KYCApproval {
uint256 kycRequirementGroup;
address user;
uint256 deadline;
}
contract BugTest1 is Test {
bytes32 APPROVAL_TYPEHASH;
bytes32 DOMAIN_SEPARATOR;
KYCRegistry registry;
address admin;
address kycAgent;
uint256 kycAgentPrivateKey = 0xB0B;
address attacker;
function setUp() public {
admin = address(0xad);
attacker = address(0xbabe);
kycAgent = vm.addr(kycAgentPrivateKey);
registry = new KYCRegistry(admin, address(new SanctionsList()));
APPROVAL_TYPEHASH = registry._APPROVAL_TYPEHASH();
DOMAIN_SEPARATOR = registry.DOMAIN_SEPARATOR();
}
function test_bug() public {
uint256 kycGroup = 1;
bytes32 kycGroupRole = "0x01";
vm.prank(admin);
registry.assignRoletoKYCGroup(kycGroup, kycGroupRole);
vm.prank(admin);
registry.grantRole(kycGroupRole, kycAgent);
vm.stopPrank();
uint256 deadline = block.timestamp + 1 days;
KYCApproval memory approval = KYCApproval({
kycRequirementGroup: kycGroup,
user: attacker,
deadline: deadline
});
bytes32 digest = getTypedDataHash(approval);
// KYC approval got signed with validity of 1 day
(uint8 v, bytes32 r, bytes32 s) = vm.sign(kycAgentPrivateKey, digest);
assertEq(registry.kycState(kycGroup, attacker), false);
assertEq(registry.getKYCStatus(kycGroup, attacker), false);
vm.prank(attacker);
registry.addKYCAddressViaSignature(kycGroup, attacker, deadline, v, r, s);
assertEq(registry.kycState(kycGroup, attacker), true);
assertEq(registry.getKYCStatus(kycGroup, attacker), true);
address[] memory toBeRemovedAddrs = new address[](1);
toBeRemovedAddrs[0] = attacker;
// KYC approval was removed
vm.prank(kycAgent);
registry.removeKYCAddresses(kycGroup, toBeRemovedAddrs);
vm.stopPrank();
assertEq(registry.getKYCStatus(kycGroup, attacker), false);
// KYC approval was granted again by replaying the original signature
vm.prank(attacker);
registry.addKYCAddressViaSignature(kycGroup, attacker, deadline, v, r, s);
assertEq(registry.kycState(kycGroup, attacker), true);
assertEq(registry.getKYCStatus(kycGroup, attacker), true);
}
function getStructHash(KYCApproval memory _approval) internal view returns (bytes32) {
return keccak256(abi.encode(APPROVAL_TYPEHASH, _approval.kycRequirementGroup, _approval.user, _approval.deadline));
}
function getTypedDataHash(KYCApproval memory _approval) public view returns (bytes32) {
return keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, getStructHash(_approval)));
}
}Recommended Mitigation Steps
A nonce mapping for message signers can be maintained; the value of which can be incremented for every successful signature validation.
mapping(address => uint) private nonces;A more detailed usage example can be found in OpenZeppelin’s EIP-2612 implementation.
ali2251 (Ondo Finance) disagreed with severity and commented:
Timestamps prevent replay attacks. These timestamps are like 30 minutes long, so the attack is valid only within 30 minutes and we can change the timestamp to 5 minutes and then it becomes exteremely hard for this attack to happen. Within 5 minutes, a suer must add themselves, then Admin removed them, then they add themselves but once 5 minutes is over, the attacker can no longer add themselves and so the admin can just remove them after 5 minutes. It can be seen here that in tests we use 9 minutes: https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/forge-tests/cash/registry/RegistrySignature.t.sol#L57
Trust (judge) decreased severity to Medium
[M-05] setPendingRedemptionBalance() may cause the user’s cash token to be lost
Submitted by bin2chen, also found by chaduke
setPendingRedemptionBalance() are not checking Old balances, resulting in the possibility of overwriting the new balance added by the user.
Proof of Concept
In setPendingRedemptionBalance(), MANAGER_ADMIN can adjust the amount of the cash token of user to be burned in some cases: addressToBurnAmt[user]
Three main parameters are passed in.
address user,
uint256 epoch,
uint256 balanceBefore modification will check epoch can not be greater than the currentEpoch, is can modify the currentEpoch user balance.
This has a problem:
The user is able to increase the addressToBurnAmt[user] of currentEpoch by requestRedemption()
This leaves open the possibility that the user may have unknowingly executed requestRedemption() before settingPendingRedemptionBalance(), causing the increased balance to be overwritten
For example:
currentEpoch = 1
Balance of alice: addressToBurnAmt[alice] = 50
- The administrator finds something wrong, there is 10 less, so he wants to increase it by 10, so he calls setPendingRedemptionBalance (balance=60)
- Alice does not know the above operation and wants to increase the redemption by 100, so it executes requestRedemption(100), which is executed earlier than setPendingRedemptionBalance() because the gas price is set higher
- The result is that the final balance of alice becomes only 60. change process:
50 => 150 => 60
The result is missing 100.
Suggest adding oldBalance, not equal will revert.
Recommended Mitigation Steps
Adding oldBalance, not equal will revert.
function setPendingRedemptionBalance(
address user,
uint256 epoch,
+ uint256 oldBalance
uint256 balance
) external updateEpoch onlyRole(MANAGER_ADMIN) {
if (epoch > currentEpoch) {
revert CannotServiceFutureEpoch();
}
+ require(oldBalance == redemptionInfoPerEpoch[epoch].addressToBurnAmt[user],"bad old balance");ali2251 (Ondo Finance) confirmed via duplicate issue #141
Low Risk and Non-Critical Issues
For this contest, 54 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by CodingNameKiki received the top score from the judge.
The following wardens also submitted reports: peanuts, luxartvinsec, 0xAgro, joestakey, Udsen, Viktor_Cortess, adriro, Aymen0909, betweenETHlines, zaskoh, csanuragjain, Josiah, 0x1f8b, rbserver, Tajobin, Kaysoft, BPZ, 0x52, cryptphi, hansfriese, 0xcm, BClabs, lukris02, nicobevi, tnevler, 0x5rings, 0xkato, erictee, tsvetanovv, IllIllI, BRONZEDISC, horsefacts, Deekshith99, gzeon, pavankv, shark, descharre, Bauer, 0xSmartContract, defsec, 2997ms, chaduke, chrisdior4, RaymondFam, scokaf, btk, arialblack14, Rolezn, Bnke0x0, koxuan, SaeedAlipoor01988, cygaar, and oyc_109.
Summary
Issues Template
| Letter | Name | Description |
|---|---|---|
| L | Low risk | Potential risk |
| NC | Non-critical | Non risky findings |
| R | Refactor | Changing the code |
| O | Ordinary | Often found issues |
| Total Found Issues | 31 |
|---|
Low Risk Issues
| Count | Explanation | Instances |
|---|---|---|
| [L-01] | Token transfer to address(0) should be avoided | 1 |
| [L-02] | The maximum fee of 10_000 isn’t allowed in the function setMintFee |
1 |
| [L-03] | Giving KYC status to address(0) should be forbidden | 1 |
| [L-04] | Redeem limit shouldn’t be set below the currentMintAmount |
1 |
| [L-05] | completeRedemptions is vulnerable to admin mistakes |
1 |
| Total Low Risk Issues | 5 |
|---|
Non-Critical Issues
| Count | Explanation | Instances |
|---|---|---|
| [N-01] | Mandatory checks for extra safety | 3 |
| [N-02] | Constructor lacks address(0) check | 5 |
| [N-03] | Missing check to ensure epoch duration isn’t set as zero seconds | 1 |
| [N-04] | Upgradeable contract is missing a __gap[50] storage variable | 2 |
| [N-05] | Create your own import names instead of using the regular ones | 19 |
| [N-06] | Initialize function does not use the initializer modifier | 2 |
| [N-07] | Use delete to clear variables instead of zero assignment | 2 |
| [N-08] | Confusing function comment on setMinimumDepositAmount |
1 |
| [N-09] | Unnecessary if statement in _checkAndUpdateRedeemLimit |
1 |
| [N-10] | Pause/Unpause functions should emit event to notify users | 2 |
| [N-11] | Two KYC checks made on the same redeemers | 1 |
| [N-12] | Unused constructor | 2 |
| [N-13] | NatSpec is incomplete in the pausing functions | 2 |
| [N-14] | Unnecessary KYC check on the payer | 1 |
| Total Non-Critical Issues | 14 |
|---|
Refactor Issues
| Count | Explanation | Instances |
|---|---|---|
| [R-01] | Shorthand way to write if / else statement | 3 |
| [R-02] | Modifier should be used instead of require on admin functions | 9 |
| [R-03] | Unused variables should be deleted | 2 |
| [R-04] | Use require instead of assert | 1 |
| [R-05] | Immutable should be used on variables that can’t be changed | 1 |
| [R-06] | Unnecessary return statement applied | 1 |
| [R-07] | Numeric values having to do with time should use time units for readability | 1 |
| Total Refactor Issues | 7 |
|---|
Ordinary Issues
| Count | Explanation | Instances |
|---|---|---|
| [O-01] | Floating pragma | 10 |
| [O-02] | Outdated Compiler Version | 3 |
| [O-03] | Function Naming suggestions | 2 |
| [O-04] | Proper use of get as a function name prefix | 2 |
| [O-05] | Events is missing indexed fields | 1 |
| Total Ordinary Issues | 5 |
|---|
[L-01] Token transfer to address(0) should be avoided
The internal function _beforeTokenTransfer ignores the use of address(0).
As how it is now the two if statements won’t be triggered on address(0) and the function will finish successfully.
56: function _beforeTokenTransfer(
57: address from,
58: address to,
59: uint256 amount
60: ) internal override {
61: super._beforeTokenTransfer(from, to, amount);
62:
63: require(
64: _getKYCStatus(_msgSender()),
65: "CashKYCSenderReceiver: must be KYC'd to initiate transfer"
66: );
67:
68: if (from != address(0)) {
69: // Only check KYC if not minting
70: require(
71: _getKYCStatus(from),
72: "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens"
73: );
74: }
75:
76: if (to != address(0)) {
77: // Only check KYC if not burning
78: require(
79: _getKYCStatus(to),
80: "CashKYCSenderReceiver: `to` address must be KYC'd to receive tokens"
81: );
82: }
83: }
84: }Consider applying a check, which will revert if the “from” or “to” address are set as the zero address and remove the two if statements:
56: function _beforeTokenTransfer(
57: address from,
58: address to,
59: uint256 amount
60: ) internal override {
61: super._beforeTokenTransfer(from, to, amount);
62:
63: require(from != address(0), "")
64: require(to != address(0), "")
65
66: require(
67: _getKYCStatus(_msgSender()),
68: "CashKYCSenderReceiver: must be KYC'd to initiate transfer"
69: );
70:
71: // Only check KYC if not minting
72: require(
73: _getKYCStatus(from),
74: "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens"
75: );
76:
77: // Only check KYC if not burning
78: require(
79: _getKYCStatus(to),
80: "CashKYCSenderReceiver: `to` address must be KYC'd to receive tokens"
81: );
83: }
84: }[L-02] Wrong if statement in the function setMintFee
The function setMintFee in CashManager.sol is used by the admin to change the mint fee.
By the dev comment above the function, the maximum fee that can be set is 10_000 bps, or 100%.
As the value of the BPSDENOMINATOR is set as `10000and can't be changed.<br> The if statement in the function is wrong as it doesn't allow to be set as the maximum fee10000bps.<br>if (mintFee >= BPSDENOMINATOR)should be changed toif (mintFee > BPS_DENOMINATOR)`.
408: // * @dev The maximum fee that can be set is 10_000 bps, or 100%contracts/cash/CashManager.sol
410: function setMintFee(
411: uint256 _mintFee
412: ) external override onlyRole(MANAGER_ADMIN) {
413: if (_mintFee >= BPS_DENOMINATOR) {
414: revert MintFeeTooLarge();
415 }
416: uint256 oldMintFee = mintFee;
417: mintFee = _mintFee;
418: emit MintFeeSet(oldMintFee, _mintFee);
419: }[L-03] Giving KYC status to address(0) should be forbidden
The function addKYCAddressViaSignature in KYCRegistry.sol is restricted to the KYC requirement group, which is allowed to give KYC status to user’s addresses. Considering the KYC status is checked all over the protocol and if KYCed the zero address can be used. A check should be made in the function addKYCAddressViaSignature to make sure
the KYC status isn’t given to the zero address.
Add this check to the function addKYCAddressViaSignature:
require(user != address(0), "KYC status for address(0) not allowed")79: function addKYCAddressViaSignature(
80: uint256 kycRequirementGroup,
81: address user,
82: uint256 deadline,
83: uint8 v,
84: bytes32 r,
85: bytes32 s
86: ) external {[L-04] Redeem limit shouldn’t be set below the currentMintAmount
The function setRedeemLimit is used by the admin to update the amount of token that can be redeemed during one epoch.
A check should be made to ensure the new redeem limit isn’t set below the currentMintAmount.
As this problem will lead to the users not being able to request redeem their minted amount of cash on the current epoch.
contracts/cash/CashManager.sol
// Before
609: function setRedeemLimit(
610: uint256 _redeemLimit
612: ) external onlyRole(MANAGER_ADMIN) {
613: uint256 oldRedeemLimit = redeemLimit;
614: redeemLimit = _redeemLimit;
615: emit RedeemLimitSet(oldRedeemLimit, _redeemLimit);
616: }
// After
609: function setRedeemLimit(
610: uint256 _redeemLimit
612: ) external onlyRole(MANAGER_ADMIN) {
613: require(_redeemLimit > currentMintAmount, "RedeemLimit below the currentMintAmount")
614: uint256 oldRedeemLimit = redeemLimit;
615: redeemLimit = _redeemLimit;
616: emit RedeemLimitSet(oldRedeemLimit, _redeemLimit);
617: }[L-05] completeRedemptions is vulnerable to admin mistakes
The function completeRedemptions allows an admin account to distribute collateral to users.
The problem is that the collateral calculation is based on the inputed spec collateralAmountToDist.
Duo to that a single admin mistake is not allowed, as it can lead to users receiving less funds back or in the worst case receiving nothing and the collateral being stuck in the assetSender contract.
contracts/cash/CashManager.sol
707: function completeRedemptions(
708: address[] calldata redeemers,
709: address[] calldata refundees,
710: uint256 collateralAmountToDist,
711: uint256 epochToService,
712: uint256 fees
713; ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
714: _checkAddressesKYC(redeemers);
715: _checkAddressesKYC(refundees);
716: if (epochToService >= currentEpoch) {
717: revert MustServicePastEpoch();
718: }
719: // Calculate the total quantity of shares tokens burned w/n an epoch
720: uint256 refundedAmt = _processRefund(refundees, epochToService);
721: uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
722: .totalBurned - refundedAmt;
723: uint256 amountToDist = collateralAmountToDist - fees;
724: _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
725: collateral.safeTransferFrom(assetSender, feeRecipient, fees);
726: emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
727: }743: function _processRedemption
755: uint256 collateralAmountDue = (amountToDist * cashAmountReturned) /
756: quantityBurned;[N-01] Mandatory checks for extra safety
In the following functions below, there are some checks that can be made in order to achieve more safe and efficient code.
- In the function
setPricea require statement can be made to check if the new price is non-zero.
contracts/lending/OndoPriceOracle.sol
80: function setPrice(address fToken, uint256 price) external override onlyOwner {
81: uint256 oldPrice = fTokenToUnderlyingPrice[fToken];
82: fTokenToUnderlyingPrice[fToken] = price;
83: emit UnderlyingPriceSet(fToken, oldPrice, price);
84: }- In the function
setFTokenToCTokena require statement can be made to check if the inputed addresses aren’t the same.
contracts/lending/OndoPriceOracle.sol
92: function setFTokenToCToken(
93: address fToken,
94: address cToken
95: ) external override onlyOwner {
96: address oldCToken = fTokenToCToken[fToken];
97: _setFTokenToCToken(fToken, cToken);
98: emit FTokenToCTokenSet(fToken, oldCToken, cToken);
99: }- In the function
setOraclea check can be made to ensure the newOracle isn’t set as address(0).
contracts/lending/OndoPriceOracle.sol
106: function setOracle(address newOracle) external override onlyOwner {
107: address oldOracle = address(cTokenOracle);
108: cTokenOracle = CTokenOracle(newOracle);
109: emit CTokenOracleSet(oldOracle, newOracle);
110: }Other instances in:
contracts/lending/OndoPriceOracleV2.sol[N-02] Constructor lacks address(0) check
Zero-address check should be used in the constructors, to avoid the risk of setting smth as address(0) at deploying time.
Instances:
contracts/cash/factory/CashFactory.sol
53: constructor(address _guardian) {
contracts/cash/factory/CashKYCSenderFactory.sol
53: constructor(address _guardian) {
contracts/cash/factory/CashKYCSenderReceiverFactory.sol
53: constructor(address _guardian) {
contracts/lending/JumpRateModelV2.sol
59: constructor() - owner
contracts/cash/kyc/KYCRegistry.sol
51: constructor() - admin[N-03] Missing check to ensure epoch duration isn’t set as zero
The function setEpochDuration is used to change the epoch duration. Considering the fact that setting an epoch’s duration as 0 seconds might lead to undesired behaviour behavior. Adding a simple check to prevent this from happening is recommended.
contracts/cash/CashManager.sol
546: function setEpochDuration(
547: uint256 _epochDuration
548: ) external onlyRole(MANAGER_ADMIN) {
549: uint256 oldEpochDuration = epochDuration;
550: epochDuration = _epochDuration;
551: emit EpochDurationSet(oldEpochDuration, _epochDuration);
552: }[N-04] Upgradeable contract is missing a __gap[50] storage variable
Reference: Storage_gaps
You may notice that every contract includes a state variable named __gap. This is empty reserved space in storage that is put in place in Upgradeable contracts. It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments.
Instances:
contracts/cash/token/CashKYCSender.sol
22: contract CashKYCSender is
23: ERC20PresetMinterPauserUpgradeable,
24: KYCRegistryClientInitializablecontracts/cash/token/Cash.sol
21: contract Cash is ERC20PresetMinterPauserUpgradeable [N-05] Create your own import names instead of using the regular ones
For better readability, you should name the imports instead of using the regular ones.
Example:
import {IKYCRegistry} from "contracts/cash/interfaces/IKYCRegistry.sol";Instances - All of the contracts.
[N-06] Initialize function does not use the initializer modifier
Without the modifier, the function may be called multiple times, overwriting prior initializations.
contracts/lending/tokens/cCash/CCash.sol
30: function initialize
contracts/lending/tokens/cToken/CErc20.sol
30: function initialize[N-07] Use delete to clear variables instead of zero assignment
You can use the delete keyword instead of setting the variable as zero.
contracts/cash/CashManager.sol
259: mintRequestsPerEpoch[epochToClaim][user] = 0
790: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[refundee] = 0[N-08] Confusing function comment on setMinimumDepositAmount
There is a little confusion between the dev comment and the if statement in the function.
As per dev comment the inputed _minimumDepositAmount should be larger than the BPS_DENOMINATOR.
But the if statement actually allows for the minimumDepositAmountto equal theBPSDENOMINATOR`.
contracts/cash/CashManager.sol
427: // @dev Must be larger than BPS_DENOMINATOR due to keep our `_getMintFees`
433: function setMinimumDepositAmount(
434: uint256 _minimumDepositAmount
435: ) external override onlyRole(MANAGER_ADMIN) {
436: if (_minimumDepositAmount < BPS_DENOMINATOR) {
437: revert MinimumDepositAmountTooSmall();
438: }
439: uint256 oldMinimumDepositAmount = minimumDepositAmount;
440: minimumDepositAmount = _minimumDepositAmount;
441: emit MinimumDepositAmountSet(
442: oldMinimumDepositAmount,
443: _minimumDepositAmount
444: );
445: }[N-09] Unnecessary if statement in _checkAndUpdateRedeemLimit
In the private function _checkAndUpdateRedeemLimit an if statement occurs, which is triggered if the inputed amount is zero.
This if statement is unnecessary and useless as there is a check already made in the core function requestRedemption to ensure the requested redemption isn’t below the minimumRedeemAmount.
contracts/cash/CashManager.sol
641: function _checkAndUpdateRedeemLimit(uint256 amount) private {
642: if (amount == 0) {
643: revert RedeemAmountCannotBeZero();
644: }
645: if (amount > redeemLimit - currentRedeemAmount) {
646: revert RedeemExceedsRateLimit();
647: }
648:
649: currentRedeemAmount += amount;
650: }
662: function requestRedemption(
663: uint256 amountCashToRedeem
664: )
665: external
666: override
667: updateEpoch
668: nonReentrant
669: whenNotPaused
670: checkKYC(msg.sender)
671: {
672: if (amountCashToRedeem < minimumRedeemAmount) {
673: revert WithdrawRequestAmountTooSmall();
674: }
675:
676: _checkAndUpdateRedeemLimit(amountCashToRedeem);[N-10] Pause/Unpause functions should emit event to notify users
The two function are used to pause and unpause the contract. Consider emitting an even to notify the users, when this is happening.
contracts/cash/CashManager.sol
526: function pause() external onlyRole(PAUSER_ADMIN) {
527: _pause();
528: }
533: function unpause() external onlyRole(MANAGER_ADMIN) {
534: _unpause();
535: }[N-11] Two KYC checks made on the same redeemers
The function completeRedemptions allows an admin account distribute collateral to users.
A check is made to ensure all of the redeemers are KYCed, but this check is unnecessary.
As in order to request redemption with the function requestRedemption, the function already check if the user calling the function is KYCed.
contracts/cash/CashManager.sol
662: function requestRedemption(
663: uint256 amountCashToRedeem
664: )
665: external
666: override
667: updateEpoch
668: nonReentrant
669: whenNotPaused
670: checkKYC(msg.sender)
707: function completeRedemptions(
708: address[] calldata redeemers,
709: address[] calldata refundees,
710: uint256 collateralAmountToDist,
711: uint256 epochToService,
712: uint256 fees
713: ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
714: _checkAddressesKYC(redeemers);[N-12] Unused constructor
The constructor does nothing.
contracts/lending/tokens/cCash/CCashDelegate.sol
15: constructor() {}
contracts/lending/tokens/cToken/CTokenDelegate.sol
15: constructor() {}[N-13] NatSpec is incomplete in the pausing functions
In the both pause and unpause functions a comment is made, that the purpose of this functions is to pause or unpause the minting functionality. This NatSpec isn’t full as it not only applies on the minting, but on the redeeming as well.
contracts/cash/CashManager.sol
522: /**
523: * @notice Will pause minting functionality of this contract
524: *
525: */
526: function pause() external onlyRole(PAUSER_ADMIN) {
527: _pause();
528: }
530: /**
531: * @notice Will unpause minting functionality of this contract
532: */
533: function unpause() external onlyRole(MANAGER_ADMIN) {
534: _unpause();
535: }
662: function requestRedemption(
663: uint256 amountCashToRedeem
664: )
665: external
666: override
667: updateEpoch
668: nonReentrant
669: whenNotPaused
670: checkKYC(msg.sender)
671: {[N-14] Unnecessary KYC check on the payer
The main use of the function repayBorrowBehalf is that a user can pay back a loan on behalf of the borrower.
The problem here is that the function repayBorrowFresh checks if both the payer and the borrower are KYCed.
In my opinion the check for the payer is unnecessary and shouldn’t be restricted like that.
As long as the borrower is KYCed, everything should be fine.
contracts/lending/tokens/cCash/CCash.sol
121: function repayBorrowBehalf(
122: address borrower,
123: uint repayAmount
124: ) external override returns (uint) {
125: repayBorrowBehalfInternal(borrower, repayAmount);
126: return NO_ERROR;
127: }
contracts/lending/tokens/cCash/CTokenCash.sol
767: function repayBorrowFresh(
768: address payer,
769: address borrower,
770: uint repayAmount
771: ) internal returns (uint) {
772: /* Revert if not KYC'd */
773: require(_getKYCStatus(payer), "Payer not KYC'd");
774: require(_getKYCStatus(borrower), "Borrower not KYC'd");[R-01] Shorthand way to write if / else statement
The normal if / else statement can be refactored in a shorthand way to write it:
- Increases readability
- Shortens the overall SLOC.
contracts/lending/OndoPriceOracle.sol
61: function getUnderlyingPrice(
62: address fToken
63: ) external view override returns (uint256) {
64: if (fTokenToUnderlyingPrice[fToken] != 0) {
65: return fTokenToUnderlyingPrice[fToken];
66: } else {
67: // Price is not manually set, attempt to retrieve price from Compound's
68: // oracle
69: address cTokenAddress = fTokenToCToken[fToken];
70: return cTokenOracle.getUnderlyingPrice(cTokenAddress);
71: }
72: } The above instance can be refactored in:
61: function getUnderlyingPrice(
62: address fToken
63: ) external view override returns (uint256) {
64: address cTokenAddress = fTokenToCToken[fToken];
65: return fTokenToUnderlyingPrice[fToken] != 0 ? fTokenToUnderlyingPrice[fToken] : cTokenOracle.getUnderlyingPrice(cTokenAddress)
66: }Other instances:
contracts/lending/tokens/cCash/CTokenCash.sol - function exchangeRateStoredInternal()
contracts/lending/tokens/cToken/CTokenModified.sol - function exchangeRateStoredInternal()[R-02] Modifier should be used instead of require on admin functions
If functions are only allowed to be called by the admin, modifier should be used instead of checking with require statement, if admin is the msg.sender calling the function.
contracts/lending/tokens/cCash/CCashDelegate.sol
21: function _becomeImplementation(bytes memory data) public virtual override {
22: // Shh -- currently unused
23: data;
24:
25: // Shh -- we don't ever want this hook to be marked pure
26: if (false) {
27: implementation = address(0);
28: }
29:
30: require(
31: msg.sender == admin,
32: "only the admin may call _becomeImplementation"
33: );
34: }Modifier should be created only accessible by the admin and the instance above can be refactored in:
21: function _becomeImplementation(bytes memory data) public virtual override onlyAdmin {
22: // Shh -- currently unused
23: data;
24:
25: // Shh -- we don't ever want this hook to be marked pure
26: if (false) {
27: implementation = address(0);
28: }
29: }Other instances:
contracts/lending/tokens/cCash/CCashDelegate.sol - function _resignImplementation()
contracts/lending/tokens/cToken/CTokenDelegate.sol - function _becomeImplementation()
contracts/lending/tokens/cToken/CTokenDelegate.sol - function _resignImplementation()
contracts/lending/tokens/cCash/CCash.sol - function sweepToken()
contracts/lending/tokens/cCash/CCash.sol - function _delegateCompLikeTo()
contracts/lending/tokens/cToken/CErc20.sol - function sweepToken()
contracts/lending/tokens/cToken/CErc20.sol - function _delegateCompLikeTo()
contracts/lending/tokens/cCash/CTokenCash.sol - function initialize()[R-03] Unused variables should be deleted
In this case bytes “data” is implemented in the function, but it isn’t used.
If not used, the variable shouldn’t be used in the first place.
contracts/lending/tokens/cCash/CCashDelegate.sol
21: function _becomeImplementation(bytes memory data) public virtual override {
22: // Shh -- currently unused
23: data;
24:
25: // Shh -- we don't ever want this hook to be marked pure
26: if (false) {
27: implementation = address(0);
28: }
29:
30: require(
31: msg.sender == admin,
32: "only the admin may call _becomeImplementation"
33: );
34: }Other instance:
contracts/lending/tokens/cToken/CTokenDelegate.sol - function _becomeImplementation()[R-04] Use require instead of assert
The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement.
contracts/cash/factory/CashFactory.sol
97: assert(cashProxyAdmin.owner() == guardian);
contracts/cash/factory/CashKYCSenderFactory.sol
106: assert(cashKYCSenderProxyAdmin.owner() == guardian);Recommended: Consider whether the condition checked in the assert() is actually an invariant. If not, replace the assert() statement with a require() statement.
[R-05] Immutable should be used on variables that can’t be changed
State variables, which can’t be changed after deploying time should be set as immutable.
contracts/lending/JumpRateModelV2.sol
24: address public owner;[R-06] Unnecessary return statement applied
Adding a return statement when the function defines a named return variable, is wrong.
contracts/cash/CashManager.sol
781: function _processRefund(
782: address[] calldata refundees,
783: uint256 epochToService
784: ) private returns (uint256 totalCashAmountRefunded) {
785: uint256 size = refundees.length;
786: for (uint256 i = 0; i < size; ++i) {
787: address refundee = refundees[i];
788: uint256 cashAmountBurned = redemptionInfoPerEpoch[epochToService]
789: .addressToBurnAmt[refundee];
790: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[refundee] = 0;
791: cash.mint(refundee, cashAmountBurned);
792: totalCashAmountRefunded += cashAmountBurned;
793: emit RefundIssued(refundee, cashAmountBurned, epochToService);
794: }
795: return totalCashAmountRefunded;
796 }[R-07] Numeric values having to do with time should use time units for readability
Suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit and units are considered naively in the following way:
1 == 1 seconds
1 minutes == 60 seconds
1 hours == 60 minutes
1 days == 24 hours
1 weeks == 7 days
contracts/lending/OndoPriceOracleV2.sol
77: uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours[O-01] Floating pragma
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
Instances:
contracts/lending/tokens/cCash/CCashDelegate.sol
contracts/lending/tokens/cToken/CTokenDelegate.sol
contracts/lending/JumpRateModelV2.sol
contracts/lending/tokens/cCash/CCash.sol
contracts/lending/tokens/cToken/CErc20.sol
contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol
contracts/lending/tokens/cToken/CTokenInterfacesModified.sol
contracts/lending/tokens/cErc20ModifiedDelegator.sol
contracts/lending/tokens/cCash/CTokenCash.sol
contracts/lending/tokens/cToken/CTokenModified.sol[O-02] Outdated Compiler Version
Using an outdated compiler version can be problematic especially if there are publicly disclosed bugs and issues that affect the current compiler version. It is recommended to use a recent version of the Solidity compiler.
Instances:
contracts/lending/OndoPriceOracle.sol
contracts/lending/JumpRateModelV2.sol
contracts/lending/tokens/cErc20ModifiedDelegator.sol[O-03] Function Naming suggestions
Proper use of _ as a function name prefix and a common pattern is to prefix internal and private function names with _.
This pattern is correctly applied in the Party contracts, however there are some inconsistencies in the libraries.
Instances:
contracts/lending/tokens/cToken/CTokenModified.sol
contracts/lending/tokens/cCash/CTokenCash.sol[O-04] Proper use of get as a function name prefix
Clear function names can increase readability. Follow a standard convertion function names such as using get for getter (view/pure) functions.
Instances:
contracts/lending/tokens/cToken/CTokenModified.sol
contracts/lending/tokens/cCash/CTokenCash.sol[O-05] Events is missing indexed fields
Index event fields make the field more quickly accessible to off-chain.
Each event should use three indexed fields if there are three or more fields.
Instances in:
contracts/lending/tokens/cErc20ModifiedDelegator.solChosen for most unique issues raised, on top of standard QA issues.
ali2251 (Ondo Finance) commented:
We will address L-02 (
The maximum fee of 10_000 isn't allowed in the function setMintFee) and L-05 (completeRedemptions is vulnerable to admin mistakes), rest are mostly intended behaviour.
Disagreements:
L-03 (Giving KYC status to address(0) should be forbidden) -> Informational
N-01 (Mandatory checks for extra safety) and N-02 (Constructor lacks address(0) check) -> Informational
Gas Optimizations
For this contest, 24 reports were submitted by wardens detailing gas optimizations. The report highlighted below by c3phas received the top score from the judge.
The following wardens also submitted reports: 0xSmartContract, SleepingBugs, adriro, Viktor_Cortess, halden, Aymen0909, 0x1f8b, cryptostellar5, dharma09, Diana, tsvetanovv, IllIllI, saneryee, pavankv, descharre, eyexploit, chaduke, RaymondFam, arialblack14, Rolezn, Bnke0x0, cygaar, and Sathish9098.
Overview
NB: Some functions have been truncated where necessary to just show affected parts of the code.
Throughout the report some places might be denoted with audit tags to show the actual place affected.
[G-01] Using immutable on variables that are only set in the constructor and never after (2.1k gas per var)
Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD.
Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas).
Total instaces: 1 gas savings 1 * 2.1k = 2.1k gas
File: /contracts/lending/JumpRateModelV2.sol
24: address public owner;diff --git a/contracts/lending/JumpRateModelV2.sol b/contracts/lending/JumpRateModelV2.sol
index a3971c6..d4f2285 100644
--- a/contracts/lending/JumpRateModelV2.sol
+++ b/contracts/lending/JumpRateModelV2.sol
@@ -21,7 +21,7 @@ contract JumpRateModelV2 is InterestRateModel {
/**
* @notice The address of the owner, i.e. the Timelock contract, which can update parameters directly
*/
- address public owner;
+ address public immutable owner;[G-02] Tightly pack storage variables/optimize the order of variable declaration (Gas Savings: 6k in total)
Here, the storage variables can be tightly packed to save some slots
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol#L13-L48
_notEnteredand admin can be packed together (Saves 1 SLOT) Gas savings: 1 * 2k = 2k
File: /contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol
//@audit: bool internal _notEntered, uint8 public decimals and address payable public admin can be packed together
contract CTokenStorage {
bool internal _notEntered;
string public name;
string public symbol;
uint8 public decimals;
uint internal constant borrowRateMaxMantissa = 0.0005e16;
uint internal constant reserveFactorMaxMantissa = 1e18;
address payable public admin;
address payable public pendingAdmin;diff --git a/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol b/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol
index dd722f4..0ce7402 100644
--- a/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol
+++ b/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol
@@ -16,6 +16,19 @@ contract CTokenStorage {
*/
bool internal _notEntered;
+ uint8 public decimals;
+ address payable public admin;
+ address payable public pendingAdmin;
string public symbol;
- uint8 public decimals;
uint internal constant borrowRateMaxMantissa = 0.0005e16;
uint internal constant borrowRateMaxMantissa = 0.0005e16;
uint internal constant reserveFactorMaxMantissa = 1e18;
- address payable public admin;
- address payable public pendingAdmin;_notEntered and admin can be packed together (Saves 1 SLOT) Gas savings: 1 * 2k = 2k
File: /contracts/lending/tokens/cToken/CTokenInterfacesModified.sol
bool internal _notEntered;
string public name;
string public symbol;
uint8 public decimals;
uint internal constant borrowRateMaxMantissa = 0.0005e16;
uint internal constant reserveFactorMaxMantissa = 1e18;
address payable public admin;diff --git a/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol b/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol
index afffb0f..58af08d 100644
--- a/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol
+++ b/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol
@@ -13,7 +13,19 @@ contract CTokenStorage {
* @dev Guard variable for re-entrancy checks
*/
bool internal _notEntered;
+ uint8 public decimals;
+ address payable public admin;
+ address payable public pendingAdmin;
string public symbol;
- uint8 public decimals;
uint internal constant borrowRateMaxMantissa = 0.0005e16;
uint internal constant reserveFactorMaxMantissa = 1e18;
- address payable public admin;
- address payable public pendingAdmin;_notEntered can be packed with an address variable(saves 1 SLOT) Gas savings: 1 * 2k = 2k
File: /contracts/lending/tokens/cErc20ModifiedDelegator.sol
//@audit: bool internal _notEntered should be packed with address payable public admin
bool internal _notEntered;
string public name;
string public symbol;
uint8 public decimals;
uint256 internal constant borrowRateMaxMantissa = 0.0005e16;
uint256 internal constant reserveFactorMaxMantissa = 1e18;
address payable public admin;diff --git a/contracts/lending/tokens/cErc20ModifiedDelegator.sol b/contracts/lending/tokens/cErc20ModifiedDelegator.sol
index c1e9170..ca1ae31 100644
--- a/contracts/lending/tokens/cErc20ModifiedDelegator.sol
+++ b/contracts/lending/tokens/cErc20ModifiedDelegator.sol
bool internal _notEntered;
+ uint8 public decimals;
+ address payable public admin;
+ address payable public pendingAdmin;
string public symbol;
- uint8 public decimals;
uint256 internal constant reserveFactorMaxMantissa = 1e18;
- address payable public admin;
- address payable public pendingAdmin;[G-03] Massive 15k per tx gas savings - use 1 and 2 for Reentrancy guard
Using true and false will trigger gas-refunds, which after London are 1/5 of what they used to be, meaning using 1 and 2 (keeping the slot non-zero), will cost 5k per change (5k + 5k) vs 20k + 5k, saving you 15k gas per function which uses the modifier.
File: /contracts/lending/tokens/cCash/CTokenCash.sol
modifier nonReentrant() {
require(_notEntered, "re-entered");
_notEntered = false;
_;
_notEntered = true; // get a gas-refund post-Istanbul
}File: /contracts/lending/tokens/cToken/CTokenModified.sol
modifier nonReentrant() {
require(_notEntered, "re-entered");
_notEntered = false;
_;
_notEntered = true; // get a gas-refund post-Istanbul
}We could debate about the above finding being on the c4udit as using bools but due to the huge impact it would have, I’ve highlighted it here. Feel free to not include it when doing gas savings calculations.
[G-04] The result of a function call should be cached rather than re-calling the function
External calls are expensive. Consider caching the following:
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cCash/CTokenCash.sol#L870-L960
CTokenCash.sol.liquidateBorrowFresh(): getBlockNumber() should be cached
File: /contracts/lending/tokens/cCash/CTokenCash.sol
870: function liquidateBorrowFresh(
889: if (accrualBlockNumber != getBlockNumber()) { //@audit: Initial call
890: revert LiquidateFreshnessCheck();
891: }
894: if (cTokenCollateral.accrualBlockNumber() != getBlockNumber()) {//@audit: 2nd call
895: revert LiquidateCollateralFreshnessCheck();
896: }diff --git a/contracts/lending/tokens/cCash/CTokenCash.sol b/contracts/lending/tokens/cCash/CTokenCash.sol
index 93d5000..5e6fc1f 100644
--- a/contracts/lending/tokens/cCash/CTokenCash.sol
+++ b/contracts/lending/tokens/cCash/CTokenCash.sol
@@ -886,12 +886,13 @@ abstract contract CTokenCash is
}
/* Verify market's block number equals current block number */
- if (accrualBlockNumber != getBlockNumber()) {
+ uint _getBlockNumber = getBlockNumber();
+ if (accrualBlockNumber != _getBlockNumber) {
revert LiquidateFreshnessCheck();
}
/* Verify cTokenCollateral market's block number equals current block number */
- if (cTokenCollateral.accrualBlockNumber() != getBlockNumber()) {
+ if (cTokenCollateral.accrualBlockNumber() != _getBlockNumber) {
revert LiquidateCollateralFreshnessCheck();
}CTokenModified.sol.liquidateBorrowFresh(): getBlockNumber() should be cached
File: /contracts/lending/tokens/cToken/CTokenModified.sol
870: function liquidateBorrowFresh(
889: if (accrualBlockNumber != getBlockNumber()) { //@audit: Initial call
890: revert LiquidateFreshnessCheck();
891: }
894: if (cTokenCollateral.accrualBlockNumber() != getBlockNumber()) {//@audit: 2nd call
895: revert LiquidateCollateralFreshnessCheck();
896: }[G-05] Cache the mapping values rather than fetch it every time
OndoPriceOracle.sol.getUnderlyingPrice(): fTokenToUnderlyingPrice[fToken] should be cached
File: /contracts/lending/OndoPriceOracle.sol
61: function getUnderlyingPrice(
62: address fToken
63: ) external view override returns (uint256) {
64: if (fTokenToUnderlyingPrice[fToken] != 0) { //@audit: Initial access
65: return fTokenToUnderlyingPrice[fToken];//@audit: 2nd access
66: } else {diff --git a/contracts/lending/OndoPriceOracle.sol b/contracts/lending/OndoPriceOracle.sol
index 471769e..ddfc781 100644
--- a/contracts/lending/OndoPriceOracle.sol
+++ b/contracts/lending/OndoPriceOracle.sol
@@ -61,8 +61,9 @@ contract OndoPriceOracle is IOndoPriceOracle, Ownable {
function getUnderlyingPrice(
address fToken
) external view override returns (uint256) {
- if (fTokenToUnderlyingPrice[fToken] != 0) {
- return fTokenToUnderlyingPrice[fToken];
+ uint256 _fTokenToUnderlyingPrice = fTokenToUnderlyingPrice[fToken];
+ if (_fTokenToUnderlyingPrice != 0) {
+ return _fTokenToUnderlyingPrice;
} else {
// Price is not manually set, attempt to retrieve price from Compound's
// oracleOndoPriceOracleV2.sol.getUnderlyingPrice(): fTokenToUnderlyingPriceCap[fToken] should be cached
File: /contracts/lending/OndoPriceOracleV2.sol
114: if (fTokenToUnderlyingPriceCap[fToken] > 0) { //@audit: Initial access
115: price = _min(price, fTokenToUnderlyingPriceCap[fToken]);//@audit: 2nd access
116: }Use the cached value here
File: /contracts/lending/tokens/cCash/CTokenCash.sol
50: // Set initial exchange rate
51: initialExchangeRateMantissa = initialExchangeRateMantissa_;//@audit: Cached here
52: require(
53: initialExchangeRateMantissa > 0, //@audit: Use the cached value
54: "initial exchange rate must be greater than zero."
55: );diff --git a/contracts/lending/tokens/cCash/CTokenCash.sol b/contracts/lending/tokens/cCash/CTokenCash.sol
index 93d5000..9f0f7da 100644
--- a/contracts/lending/tokens/cCash/CTokenCash.sol
+++ b/contracts/lending/tokens/cCash/CTokenCash.sol
@@ -50,7 +50,7 @@ abstract contract CTokenCash is
// Set initial exchange rate
initialExchangeRateMantissa = initialExchangeRateMantissa_;
require(
- initialExchangeRateMantissa > 0,
+ initialExchangeRateMantissa_ > 0,
"initial exchange rate must be greater than zero."
);[G-06] Internal/Private functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Affected code:
File: /contracts/lending/OndoPriceOracle.sol
119: function _setFTokenToCToken(address fToken, address cToken) internal {File: /contracts/lending/OndoPriceOracleV2.sol
210: function _setFTokenToCToken(address fToken, address cToken) internal {
251: function _setFTokenToChainlinkOracle(
252: address fToken,
253: address chainlinkOracle
254: ) internal {
324: function _min(uint256 a, uint256 b) internal pure returns (uint256) {[G-07] Multiple accesses of a mapping/array should use a local variable cache
Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.
Help the Optimizer by saving a storage variable’s reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
CashManager.sol.requestRedemption(): redemptionInfoPerEpoch[currentEpoch] should be cached in storage
File:/contracts/cash/CashManager.sol
662: function requestRedemption(
678: redemptionInfoPerEpoch[currentEpoch].addressToBurnAmt[
679: msg.sender
680: ] += amountCashToRedeem;
681: redemptionInfoPerEpoch[currentEpoch].totalBurned += amountCashToRedeem;CashManager.sol.setPendingRedemptionBalance(): redemptionInfoPerEpoch[epoch] should be cached in local storage
File:/contracts/cash/CashManager.sol
851: function setPendingRedemptionBalance(
859: uint256 previousBalance = redemptionInfoPerEpoch[epoch].addressToBurnAmt[
860: user
861: ];
864: if (balance < previousBalance) {
865: redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance;
866: } else if (balance > previousBalance) {
867: redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance;
868: }
869: redemptionInfoPerEpoch[epoch].addressToBurnAmt[user] = balance;
870: emit PendingRedemptionBalanceSet(
871: user,
872: epoch,
873: balance,
874: redemptionInfoPerEpoch[epoch].totalBurned
875: );
876: }CTokenCash.sol.borrowFresh(): accountBorrows[borrower] should be cached in local storage
File: /contracts/lending/tokens/cCash/CTokenCash.sol
720: accountBorrows[borrower].principal = accountBorrowsNew;//@audit: Initial access
721: accountBorrows[borrower].interestIndex = borrowIndex;//@audit: 2nd accessCTokenCash.sol.repayBorrowFresh(): accountBorrows[borrower] should be cached in local storage
File: /contracts/lending/tokens/cCash/CTokenCash.sol
822: accountBorrows[borrower].principal = accountBorrowsNew; //@audit: Initial access
823: accountBorrows[borrower].interestIndex = borrowIndex;//@audit: 2nd accessCTokenModified.sol.borrowFresh(): accountBorrows[borrower] should be cached in local storage
File:/contracts/lending/tokens/cToken/CTokenModified.sol
720: accountBorrows[borrower].principal = accountBorrowsNew;//@audit: Initial access
721: accountBorrows[borrower].interestIndex = borrowIndex;//@audit: 2nd accessCTokenModified.sol.repayBorrowFresh(): accountBorrows[borrower] should be cached in local storage
File:/contracts/lending/tokens/cToken/CTokenModified.sol
822: accountBorrows[borrower].principal = accountBorrowsNew;//@audit: Initial access
823: accountBorrows[borrower].interestIndex = borrowIndex;//@audit: 2nd accessOndoPriceOracleV2.sol._setFTokenToChainlinkOracle(): fTokenToChainlinkOracle[fToken] should be cached in local storage
File: /contracts/lending/OndoPriceOracleV2.sol
251: function _setFTokenToChainlinkOracle(
260: fTokenToChainlinkOracle[fToken].scaleFactor = (10 **
261: (36 -
262: uint256(IERC20Like(underlying).decimals()) -
263: uint256(AggregatorV3Interface(chainlinkOracle).decimals()))); //@audit: Initial access
264: fTokenToChainlinkOracle[fToken].oracle = AggregatorV3Interface(
265: chainlinkOracle
266: );//@audit: 2nd access
267: }[G-08] Emitting storage values instead of the memory one.
Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead:
File: /contracts/lending/JumpRateModelV2.sol
171: function updateJumpRateModelInternal(
182: kink = kink_;
184: emit NewInterestParams(
185: baseRatePerBlock,
186: multiplierPerBlock,
187: jumpMultiplierPerBlock,
188: kink
189: );
190: }diff --git a/contracts/lending/JumpRateModelV2.sol b/contracts/lending/JumpRateModelV2.sol
index a3971c6..cc525bb 100644
--- a/contracts/lending/JumpRateModelV2.sol
+++ b/contracts/lending/JumpRateModelV2.sol
@@ -185,7 +185,7 @@ contract JumpRateModelV2 is InterestRateModel {
baseRatePerBlock,
multiplierPerBlock,
jumpMultiplierPerBlock,
- kink
+ kink_
);
}
}Other instances
File: /contracts/cash/CashManager.sol
//@audit: we should emit newRedeemMinimum instead of minimumRedeemAmount
817: function setRedeemMinimum(
818: uint256 newRedeemMinimum
819: ) external onlyRole(MANAGER_ADMIN) {
820: uint256 oldRedeemMin = minimumRedeemAmount;
821: minimumRedeemAmount = newRedeemMinimum;
822: emit MinimumRedeemAmountSet(oldRedeemMin, minimumRedeemAmount);
823: }File: /contracts/lending/tokens/cErc20ModifiedDelegator.sol
//@audit: We should emit implementation_ instead of implementation
720: function _setImplementation(
721: address implementation_,
722: bool allowResign,
723: bytes memory becomeImplementationData
724: ) public {
736: address oldImplementation = implementation;
737: implementation = implementation_;
746: emit NewImplementation(oldImplementation, implementation);
747: }[G-09] Using storage instead of memory for structs/arrays saves gas
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
File: /contracts/lending/OndoPriceOracleV2.sol
284: ChainlinkOracleInfo memory chainlinkInfo = fTokenToChainlinkOracle[fToken];[G-10] Refactor the code here to avoid storage readings
Note: I’ve added some explanations as to how/why this would work
1st instance
File: /contracts/lending/tokens/cToken/CTokenModified.sol
function _reduceReservesFresh(uint reduceAmount) internal returns (uint) {
// Check caller is admin
if (msg.sender != admin) {
revert ReduceReservesAdminCheck();
}
doTransferOut(admin, reduceAmount);
emit ReservesReduced(admin, reduceAmount, totalReservesNew);The only way we get to doTransferOut(admin, reduceAmount); is if the msg.sender is equal to admin, therefore rather than use admin (storage variable) in the function call doTransferOut(admin, reduceAmount); we could use the cheaper msg.sender. Similar to the emit line , we could just emit msg.sender
diff --git a/contracts/lending/tokens/cToken/CTokenModified.sol b/contracts/lending/tokens/cToken/CTokenModified.sol
index 8798b90..45b24da 100644
--- a/contracts/lending/tokens/cToken/CTokenModified.sol
+++ b/contracts/lending/tokens/cToken/CTokenModified.sol
@@ -1287,9 +1287,9 @@ abstract contract CTokenModified is
totalReserves = totalReservesNew;
// doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
- doTransferOut(admin, reduceAmount);
+ doTransferOut(payable(msg.sender), reduceAmount);
- emit ReservesReduced(admin, reduceAmount, totalReservesNew);
+ emit ReservesReduced(msg.sender, reduceAmount, totalReservesNew);
return NO_ERROR;
}File: /contracts/lending/tokens/cCash/CTokenCash.sol
1253: function _reduceReservesFresh(uint reduceAmount) internal returns (uint) {
1257: // Check caller is admin
1258: if (msg.sender != admin) {
1259: revert ReduceReservesAdminCheck();
1260: }
1287: doTransferOut(admin, reduceAmount);//@audit: use msg.sender here instead of admin
1289: emit ReservesReduced(admin, reduceAmount, totalReservesNew);//@audit: use msg.sender here File:/contracts/lending/tokens/cCash/CCash.sol
150: function sweepToken(EIP20NonStandardInterface token) external override {
151: require(
152: msg.sender == admin,
153: "cErc20::sweepToken: only admin can sweep tokens"
154: );
160: token.transfer(admin, balance);
161: }Since we are checking that msg.sender == admin, it means the only way we get to line 160 is if the two are equal thus we can just use msg.sender(global variable - cheaper) in token.transfer(admin, balance); rather than use admin(storage variable - expensive).
diff --git a/contracts/lending/tokens/cCash/CCash.sol b/contracts/lending/tokens/cCash/CCash.sol
index 996283d..9f83506 100644
--- a/contracts/lending/tokens/cCash/CCash.sol
+++ b/contracts/lending/tokens/cCash/CCash.sol
@@ -157,7 +157,7 @@ contract CCash is CTokenCash, CErc20Interface {
"cErc20::sweepToken: can not sweep underlying token"
);
uint256 balance = token.balanceOf(address(this));
- token.transfer(admin, balance);
+ token.transfer(payable(msg.sender), balance);
}File: /contracts/lending/tokens/cCash/CCash.sol
30: function initialize(
53: // Set underlying and sanity check it
54: underlying = underlying_;
55: EIP20Interface(underlying).totalSupply();
56: }As underlying (storage variable) is equal to underlying_ (local variable) it would be cheaper to just read the local variable here EIP20Interface(underlying).totalSupply();
diff --git a/contracts/lending/tokens/cCash/CCash.sol b/contracts/lending/tokens/cCash/CCash.sol
index 996283d..8e30298 100644
--- a/contracts/lending/tokens/cCash/CCash.sol
+++ b/contracts/lending/tokens/cCash/CCash.sol
@@ -52,7 +52,7 @@ contract CCash is CTokenCash, CErc20Interface {
// Set underlying and sanity check it
underlying = underlying_;
- EIP20Interface(underlying).totalSupply();
+ EIP20Interface(underlying_).totalSupply();
}File: /contracts/lending/tokens/cToken/CErc20.sol
30: function initialize(
53: // Set underlying and sanity check it
54: underlying = underlying_;
55: EIP20Interface(underlying).totalSupply();
56: }diff --git a/contracts/lending/tokens/cToken/CErc20.sol b/contracts/lending/tokens/cToken/CErc20.sol
index 6998c56..3bb0011 100644
--- a/contracts/lending/tokens/cToken/CErc20.sol
+++ b/contracts/lending/tokens/cToken/CErc20.sol
@@ -52,7 +52,7 @@ contract CErc20 is CTokenModified, CErc20Interface {
// Set underlying and sanity check it
underlying = underlying_;
- EIP20Interface(underlying).totalSupply();
+ EIP20Interface(underlying_).totalSupply();
}File:/contracts/lending/tokens/cToken/CErc20.sol
150: function sweepToken(EIP20NonStandardInterface token) external override {
151: require(
152: msg.sender == admin,
153: "cErc20::sweepToken: only admin can sweep tokens"
154: );
160: token.transfer(admin, balance);
161: }The require statement ensures that msg.sender == admin therefore we can use msg.sender (global variable - cheap) in token.transfer(admin, balance) rather than admin (storage - expensive)
diff --git a/contracts/lending/tokens/cToken/CErc20.sol b/contracts/lending/tokens/cToken/CErc20.sol
index 6998c56..2684cbf 100644
--- a/contracts/lending/tokens/cToken/CErc20.sol
+++ b/contracts/lending/tokens/cToken/CErc20.sol
@@ -157,7 +157,7 @@ contract CErc20 is CTokenModified, CErc20Interface {
"cErc20::sweepToken: can not sweep underlying token"
);
uint256 balance = token.balanceOf(address(this));
- token.transfer(admin, balance);
+ token.transfer(payable(msg.sender), balance);
}[G-11] Duplicated require()/revert() checks should be refactored to a modifier or function
This saves deployment gas.
File: /contracts/lending/tokens/cCash/CCash.sol
151: require(
152: msg.sender == admin,
153: "cErc20::sweepToken: only admin can sweep tokens"
154: );The above check is also repeated on Line 269
File: /contracts/lending/tokens/cToken/CErc20.sol
151: require(
152: msg.sender == admin,
153: "cErc20::sweepToken: only admin can sweep tokens"
154: );Repeated on the following:
File: /contracts/lending/tokens/cCash/CCashDelegate.sol
30: require(
31: msg.sender == admin,
32: "only the admin may call _becomeImplementation"
33: );
45: require(
46: msg.sender == admin,
47: "only the admin may call _resignImplementation"
48: );File: /contracts/lending/tokens/cToken/CTokenDelegate.sol
30: require(
31: msg.sender == admin,
32: "only the admin may call _becomeImplementation"
33: );
45: require(
46: msg.sender == admin,
47: "only the admin may call _resignImplementation"
48: );File: /contracts/lending/tokens/cCash/CTokenCash.sol
44: require(msg.sender == admin, "only admin may initialize the market");We can have a modifier that checks that the msg.sender is the admin. Unless we really need to have different error messages we could generalize the errors for all functions that expect to be called by an admin.
The above check or a variation of it is found on the following lines
File: /contracts/lending/tokens/cCash/CTokenCash.sol
1064: if (msg.sender != admin) {
1065: revert SetPendingAdminOwnerCheck();
1066: }
1116: if (msg.sender != admin) {
1117: revert SetComptrollerOwnerCheck();
1118: }
1155: if (msg.sender != admin) {
1156: revert SetReserveFactorAdminCheck();
1157: }
1258: if (msg.sender != admin) {
1259: revert ReduceReservesAdminCheck();
1260: }
1321: if (msg.sender != admin) {
1322: revert SetInterestRateModelOwnerCheck();
1323: }
1357: require(msg.sender == admin, "Only admin can set KYC requirement group");
1379: require(msg.sender == admin, "Only admin can set KYC registry");File: /contracts/lending/tokens/cCash/CTokenCash.sol
44: require(msg.sender == admin, "only admin may initialize the market");
1064: if (msg.sender != admin) {
1065: revert SetPendingAdminOwnerCheck();
1066: }
1116: if (msg.sender != admin) {
1117: revert SetComptrollerOwnerCheck();
1118: }
1155: if (msg.sender != admin) {
1156: revert SetReserveFactorAdminCheck();
1157: }
1261: if (msg.sender != admin) {
1262: revert ReduceReservesAdminCheck();
1263: }
1324: if (msg.sender != admin) {
1325: revert SetInterestRateModelOwnerCheck();
1326: }
1360: require(msg.sender == admin, "Only admin can set KYC requirement group");
1382: require(msg.sender == admin, "Only admin can set KYC registry");[G-12] x += y costs more gas than x = x + y for state variables
Saves 24 Gas on average
| Min | Average | Median | Max | |
|---|---|---|---|---|
| Before | 1176 | 1420 | 1469 | 1469 |
| After | 1156 | 1396 | 1444 | 1444 |
File: /contracts/cash/CashManager.sol
582: currentEpoch += epochDifference;diff --git a/contracts/cash/CashManager.sol b/contracts/cash/CashManager.sol
index 4eb4203..3156721 100644
--- a/contracts/cash/CashManager.sol
+++ b/contracts/cash/CashManager.sol
@@ -579,7 +579,7 @@ contract CashManager is
if (epochDifference > 0) {
currentRedeemAmount = 0;
currentMintAmount = 0;
- currentEpoch += epochDifference;
+ currentEpoch = currentEpoch + epochDifference;
currentEpochStartTimestamp =
block.timestamp -
(block.timestamp % epochDuration);File: /contracts/cash/CashManager.sol
630: currentMintAmount += collateralAmountIn;
649: currentRedeemAmount += amount;[G-13] Using unchecked blocks to save gas
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.
see resource
File: /contracts/cash/CashManager.sol
297: rateDifference = exchangeRate - lastSetMintExchangeRate;The operation exchangeRate - lastSetMintExchangeRate cannot underflow as it would only be evaluated if exchangeRate is greater than lastSetMintExchangeRate
We can modify it as follows
diff --git a/contracts/cash/CashManager.sol b/contracts/cash/CashManager.sol
index 4eb4203..a99cea7 100644
--- a/contracts/cash/CashManager.sol
+++ b/contracts/cash/CashManager.sol
@@ -294,7 +294,10 @@ contract CashManager is
uint256 rateDifference;
if (exchangeRate > lastSetMintExchangeRate) {
- rateDifference = exchangeRate - lastSetMintExchangeRate;
+ unchecked {
+ rateDifference = exchangeRate - lastSetMintExchangeRate;
+ }
+
} File: /contracts/cash/CashManager.sol
299: rateDifference = lastSetMintExchangeRate - exchangeRate;The operation lastSetMintExchangeRate - exchangeRate cannot underflow as it would only be evaluated if lastSetMintExchangeRate is greater than exchangeRate.
File: /contracts/cash/CashManager.sol
865: redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance;The operation previousBalance - balance cannot underflow as it would only be evaluated if previousBalance is greater than balance due to the check on Line 864.
File: /contracts/cash/CashManager.sol
867: redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance;The operation balance - previousBalance cannot underflow as it would only be evaluated if balance is greater than previousBalance due to the check on Line 866.
File: /contracts/cash/CashManager.sol
1281: totalReservesNew = totalReserves - reduceAmount;The operation totalReserves - reduceAmount cannot underflow due to the check on Line 1273 that ensures that totalReserves is greater than reduceAmount before performing the arithemtic operation.
File: /contracts/cash/CashManager.sol
1284: totalReservesNew = totalReserves - reduceAmount;The operation totalReserves - reduceAmount cannot underflow due to the check on Line 1276 that ensures that totalReserves is greater than reduceAmount before performing the arithemtic operation.
[G-14] Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)
The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.
e.g Let’s work with a sample loop below.
for(uint256 i; i < 10; i++){
//doSomething
}can be written as shown below.
for(uint256 i; i < 10;) {
// loop logic
unchecked { i++; }
}We can also write it as an inlined function like below.
function inc(i) internal pure returns (uint256) {
unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
// doSomething
}File: /contracts/cash/factory/CashFactory.sol
127: for (uint256 i = 0; i < exCallData.length; ++i) {
128: (bool success, bytes memory ret) = address(exCallData[i].target).call{
129: value: exCallData[i].value
130: }(exCallData[i].data);
131: require(success, "Call Failed");
132: results[i] = ret;
133: }The above should be modified to:
diff --git a/contracts/cash/factory/CashFactory.sol b/contracts/cash/factory/CashFactory.sol
index 24b67ba..84bb244 100644
--- a/contracts/cash/factory/CashFactory.sol
+++ b/contracts/cash/factory/CashFactory.sol
@@ -124,12 +124,15 @@ contract CashFactory is IMulticall {
ExCallData[] calldata exCallData
) external payable override onlyGuardian returns (bytes[] memory results) {
results = new bytes[](exCallData.length);
- for (uint256 i = 0; i < exCallData.length; ++i) {
+ for (uint256 i = 0; i < exCallData.length;) {
(bool success, bytes memory ret) = address(exCallData[i].target).call{
value: exCallData[i].value
}(exCallData[i].data);
require(success, "Call Failed");
results[i] = ret;
+ unchecked {
+ ++i;
+ }
}
}Other Instances to modify
File: /contracts/cash/factory/CashKYCSenderFactory.sol
137: for (uint256 i = 0; i < exCallData.length; ++i) {File: /contracts/cash/factory/CashKYCSenderReceiverFactory.sol
137: for (uint256 i = 0; i < exCallData.length; ++i) {File: /contracts/cash/kyc/KYCRegistry.sol
163: for (uint256 i = 0; i < length; i++) {
180: for (uint256 i = 0; i < length; i++) {File: /contracts/cash/CashManager.sol
750: for (uint256 i = 0; i < size; ++i) {
786: for (uint256 i = 0; i < size; ++i) {
933: for (uint256 i = 0; i < size; ++i) {
961: for (uint256 i = 0; i < exCallData.length; ++i) {[G-15] Splitting require() statements that use && saves gas - (saves 8 gas per &&)
Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per &&.
The gas difference would only be realized if the revert condition is realized(met).
Proof of Concept
The following tests were carried out in remix with both optimization turned on and off
function multiple (uint a) public pure returns (uint){
require ( a > 1 && a < 5, "Initialized");
return a + 2;
}Execution cost
21617 with optimization and using &&
21976 without optimization and using &&
After splitting the require statement
function multiple(uint a) public pure returns (uint){
require (a > 1 ,"Initialized");
require (a < 5 , "Initialized");
return a + 2;
}Execution cost
21609 with optimization and split require
21968 without optimization and using split require
File: /contracts/lending/OndoPriceOracleV2.sol
292: require(
293: (answeredInRound >= roundId) &&
294: (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay),
295: "Chainlink oracle price is stale"
296: );File: /contracts/lending/tokens/cCash/CTokenCash.sol
45: require(
46: accrualBlockNumber == 0 && borrowIndex == 0,
47: "market may only be initialized once"
48: );[G-16] Reorder the require statements to have the less gas consuming before the expensive one
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/kyc/KYCRegistry.sol#L79-L112
Save 225 gas on average
| | Min | Average | Median | Max |
| ------ | --- | ------- | ----- | ----- |
| Before | 752 | 23892 | 34436 | 41128 |
| After | 690 | 23667 | 34436 | 41128 |
File: /contracts/cash/kyc/KYCRegistry.sol
79: function addKYCAddressViaSignature(
80: uint256 kycRequirementGroup,
81: address user,
82: uint256 deadline,
83: uint8 v,
84: bytes32 r,
85: bytes32 s
86: ) external {
87: require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
88: require(
89: !kycState[kycRequirementGroup][user],
90: "KYCRegistry: user already verified"
91: );
92: require(block.timestamp <= deadline, "KYCRegistry: signature expired");Its cheaper to check for block.timestamp <= deadline as compared to !kycState\[kycRequirementGroup]\[user] as this involves reading the storage variable. Therefore if the require(block.timestamp <= deadline, "KYCRegistry: signature expired"); fails it would be cheaper to fail before evaluating the !kycState\[kycRequirementGroup]\[user]
diff --git a/contracts/cash/kyc/KYCRegistry.sol b/contracts/cash/kyc/KYCRegistry.sol
index 896c727..d5401df 100644
--- a/contracts/cash/kyc/KYCRegistry.sol
+++ b/contracts/cash/kyc/KYCRegistry.sol
@@ -85,11 +85,12 @@ contract KYCRegistry is AccessControlEnumerable, IKYCRegistry, EIP712 {
bytes32 s
) external {
require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
+ require(block.timestamp <= deadline, "KYCRegistry: signature expired");
+
require(
!kycState[kycRequirementGroup][user],
"KYCRegistry: user already verified"
);
- require(block.timestamp <= deadline, "KYCRegistry: signature expired");
bytes32 structHash = keccak256(
abi.encode(_APPROVAL_TYPEHASH, kycRequirementGroup, user, deadline)
);[G-17] Caching global variables is more expensive than using the actual variable(use msg.sender instead of caching it)
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cCash/CTokenCash.sol#L182-L190
It’s cheaper to use msg.sender as compared to caching
File: /contracts/lending/tokens/cCash/CTokenCash.sol
182: function approve(
183: address spender,
184: uint256 amount
185: ) external override returns (bool) {
186: address src = msg.sender;
187: transferAllowances[src][spender] = amount;
188: emit Approval(src, spender, amount);
189: return true;
190: }diff --git a/contracts/lending/tokens/cCash/CTokenCash.sol b/contracts/lending/tokens/cCash/CTokenCash.sol
index 93d5000..2dbaadd 100644
--- a/contracts/lending/tokens/cCash/CTokenCash.sol
+++ b/contracts/lending/tokens/cCash/CTokenCash.sol
@@ -183,9 +183,8 @@ abstract contract CTokenCash is
address spender,
uint256 amount
) external override returns (bool) {
- address src = msg.sender;
- transferAllowances[src][spender] = amount;
- emit Approval(src, spender, amount);
+ transferAllowances[msg.sender][spender] = amount;
+ emit Approval(msg.sender, spender, amount);
return true;
}[G-18] Use a more recent version of solidity
Use a solidity version of at least 0.8 to get default underflow/overflow checks, use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.
We can avoid using the library safeMath in the following file by using version 0.8+
File: /contracts/lending/JumpRateModelV2.sol
1:pragma solidity ^0.5.16;ypatil12 (Ondo Finance) commented:
Amazing report.
Disclosures
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest 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.