Back to matrix_0wl's contests

Ondo Finance - matrix_0wl's results

Institutional-Grade Finance. On-Chain. For Everyone.

General Information

Platform: Code4rena

Start Date: 01/09/2023

Pot Size: $36,500 USDC

Total HM: 4

Participants: 70

Period: 6 days

Judge: kirk-baird

Id: 281

League: ETH

Ondo Finance

Findings Distribution

Researcher Performance

Rank: 34/70

Findings: 2

Award: $97.33

QA:
grade-a
Gas:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryOndo Finance

Findings Information

🌟 Selected for report: adriro

Also found by: 0x6980, 0xStalin, 0xanmol, 0xmystery, 0xpanicError, Arz, Aymen0909, BenRai, Breeje, Lalanda, MohammedRizwan, Raihan, SovaSlava, Stormreckson, Udsen, ast3ros, bin2chen, castle_chain, catellatech, codegpt, dev0cloo, gkrastenov, hals, klau5, kutugu, ladboy233, matrix_0wl, nirlin, ohm, peanuts, pipidu83, sandy, wahedtalash77

Awards

87.5807 USDC - $87.58

Labels

bug
grade-a
QA (Quality Assurance)
sponsor disputed
sufficient quality report
Q-34

External Links

Link to GitHub issue

Non Critical Issues

Issue
NC-1ADD A TIMELOCK TO CRITICAL FUNCTIONS
NC-2ADD TO BLACKLIST FUNCTION
NC-3GENERATE PERFECT CODE HEADERS EVERY TIME
NC-4SAME CONSTANT REDEFINED ELSEWHERE
NC-5IMPLEMENTATION CONTRACT MAY NOT BE INITIALIZED
NC-6MARK VISIBILITY OF INITIALIZE(…) FUNCTIONS AS EXTERNAL
NC-7CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT
NC-8LACK OF EVENT EMISSION AFTER CRITICAL INITIALIZE() FUNCTIONS
NC-9FOR EXTENDED “USING-FOR” USAGE, USE THE LATEST PRAGMA VERSION
NC-10NO SAME VALUE INPUT CONTROL
NC-11Add parameter to event-emit
NC-12SOLIDITY COMPILER OPTIMIZATIONS CAN BE PROBLEMATIC
NC-13LINES ARE TOO LONG

[NC-1] ADD A TIMELOCK TO CRITICAL FUNCTIONS

Description:

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

255:   function setThresholds(

286:   function setMintLimit(uint256 mintLimit) external onlyOwner {

287:     _setMintLimit(mintLimit);

295:   function setMintLimitDuration(uint256 mintDuration) external onlyOwner {

296:     _setMintLimitDuration(mintDuration);

343:         ALLOWLIST.setAccountStatus(

File: contracts/bridge/SourceBridge.sol

121:   function setDestinationChainContractAddress(

File: contracts/rwaOracles/RWADynamicOracle.sol

32:     address setter,

41:     _grantRole(SETTER_ROLE, setter);

151:   function setRange(

File: contracts/usdy/rUSDY.sol

662:   function setOracle(address _oracle) external onlyRole(USDY_MANAGER_ROLE) {

698:   function setBlocklist(

701:     _setBlocklist(blocklist);

709:   function setAllowlist(

712:     _setAllowlist(allowlist);

720:   function setSanctionsList(

723:     _setSanctionsList(sanctionsList);

[NC-2] ADD TO BLACKLIST FUNCTION

Description:

NFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into liquidity. To prevent this, I recommend adding the blacklist function.

Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.

Here is the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

Add to Blacklist function and modifier.

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

[NC-3] GENERATE PERFECT CODE HEADERS EVERY TIME

Description:

I recommend using header for Solidity code layout and readability: https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

[NC-4] SAME CONSTANT REDEFINED ELSEWHERE

Description:

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values).

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

48:   bytes32 public constant VERSION = "1.0";

File: contracts/bridge/SourceBridge.sol

27:   bytes32 public constant VERSION = "1.0";

File: contracts/rwaOracles/RWADynamicOracle.sol

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

[NC-5] IMPLEMENTATION CONTRACT MAY NOT BE INITIALIZED

Description:

OpenZeppelin recommends that the initializer modifier be applied to constructors.

Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits.

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

Proof Of Concept
File: contracts/usdy/rUSDY.sol

20: import "contracts/external/openzeppelin/contracts-upgradeable/proxy/Initializable.sol";

58:   Initializable,

[NC-6] MARK VISIBILITY OF INITIALIZE(…) FUNCTIONS AS EXTERNAL

Description:

If someone wants to extend via inheritance, it might make more sense that the overridden initialize(...) function calls the internal {...}_init function, not the parent public initialize(...) function.

External instead of public would give more sense of the initialize(...) functions to behave like a constructor (only called on deployment, so should only be called externally).

From a security point of view, it might be safer so that it cannot be called internally by accident in the child contract.

Proof Of Concept
File: contracts/usdy/rUSDY.sol

109:   function initialize(

File: contracts/usdy/rUSDYFactory.sol

90:     rUSDYProxied.initialize(

[NC-7] CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Description:

There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts.

While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand.

WConstants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

27:   bytes32 public constant SETTER_ROLE = keccak256("SETTER_ROLE");

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

97:   bytes32 public constant USDY_MANAGER_ROLE = keccak256("ADMIN_ROLE");

98:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

100:   bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE");

[NC-8] LACK OF EVENT EMISSION AFTER CRITICAL INITIALIZE() FUNCTIONS

Description:

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions.

Proof Of Concept
File: contracts/usdy/rUSDY.sol

109:   function initialize(

File: contracts/usdy/rUSDYFactory.sol

90:     rUSDYProxied.initialize(

[NC-9] FOR EXTENDED “USING-FOR” USAGE, USE THE LATEST PRAGMA VERSION

Description:

https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

16: pragma solidity 0.8.16;

File: contracts/bridge/SourceBridge.sol

1: pragma solidity 0.8.16;

File: contracts/rwaOracles/IRWADynamicOracle.sol

16: pragma solidity 0.8.16;

File: contracts/rwaOracles/RWADynamicOracle.sol

16: pragma solidity 0.8.16;

File: contracts/usdy/rUSDY.sol

16: pragma solidity 0.8.16;

File: contracts/usdy/rUSDYFactory.sol

16: pragma solidity 0.8.16;

[NC-10] NO SAME VALUE INPUT CONTROL

Proof Of Concept
File: contracts/usdy/rUSDYFactory.sol

52:     guardian = _guardian;

[NC-11] Add parameter to event-emit

Description:

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters.

The events should include the new value and old value where possible.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

212:     emit ApproverAdded(approver);

222:     emit ApproverRemoved(approver);

[NC-12] SOLIDITY COMPILER OPTIMIZATIONS CAN BE PROBLEMATIC

Description:

Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.

Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.

Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.

Exploit Scenario:

A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Proof Of Concept
File: hardhat.config.ts

 solidity: {
    compilers: [
      {
        version: "0.8.16",
        settings: {
          optimizer: {
            enabled: true,
            runs: 100,
          },
        },
      },
      {
        version: "0.7.0",
        settings: {
          optimizer: {
            enabled: true,
            runs: 100,
          },
        },
      },
      {
        version: "0.6.12",
        settings: {
          optimizer: {
            enabled: true,
            runs: 100,
          },
        },
      },
      {
        version: "0.4.24",
        settings: {
          optimizer: {
            enabled: true,
            runs: 200,
          },
        },
      },
    ],
  },

Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug.

Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[NC-13] LINES ARE TOO LONG

Description:

Usually lines in source code are limited to 80 characters. Today’s screens are much larger so it’s reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.

Reference

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

7: ██  ██ ╒█▀'   ╙█▌ ╙█▌ ██     ▐██      ███  █████,  ██  ██▌    └██▌  ██▌     └██▌

8: ██ ▐█▌ ██      ╟█  █▌ ╟█     ██▌      ▐██  ██ └███ ██  ██▌     ╟██ j██       ╟██

9: ╟█  ██ ╙██    ▄█▀ ▐█▌ ██     ╙██      ██▌  ██   ╙████  ██▌    ▄██▀  ██▌     ,██▀

238:     chainToApprovedSender[srcChain] = keccak256(abi.encode(srcContractAddress));

414:   event ThresholdSet(string chain, uint256[] amounts, uint256[] numOfApprovers);

File: contracts/bridge/SourceBridge.sol

7: import {AddressToString} from "contracts/external/axelar/StringAddressUtils.sol";

File: contracts/rwaOracles/IRWADynamicOracle.sol

7: ██  ██ ╒█▀'   ╙█▌ ╙█▌ ██     ▐██      ███  █████,  ██  ██▌    └██▌  ██▌     └██▌

8: ██ ▐█▌ ██      ╟█  █▌ ╟█     ██▌      ▐██  ██ └███ ██  ██▌     ╟██ j██       ╟██

9: ╟█  ██ ╙██    ▄█▀ ▐█▌ ██     ╙██      ██▌  ██   ╙████  ██▌    ▄██▀  ██▌     ,██▀

File: contracts/rwaOracles/RWADynamicOracle.sol

7: ██  ██ ╒█▀'   ╙█▌ ╙█▌ ██     ▐██      ███  █████,  ██  ██▌    └██▌  ██▌     └██▌

8: ██ ▐█▌ ██      ╟█  █▌ ╟█     ██▌      ▐██  ██ └███ ██  ██▌     ╟██ j██       ╟██

9: ╟█  ██ ╙██    ▄█▀ ▐█▌ ██     ╙██      ██▌  ██   ╙████  ██▌    ▄██▀  ██▌     ,██▀

19: import "contracts/external/openzeppelin/contracts/access/AccessControlEnumerable.sol";

File: contracts/usdy/rUSDY.sol

7: ██  ██ ╒█▀'   ╙█▌ ╙█▌ ██     ▐██      ███  █████,  ██  ██▌    └██▌  ██▌     └██▌

8: ██ ▐█▌ ██      ╟█  █▌ ╟█     ██▌      ▐██  ██ └███ ██  ██▌     ╟██ j██       ╟██

9: ╟█  ██ ╙██    ▄█▀ ▐█▌ ██     ╙██      ██▌  ██   ╙████  ██▌    ▄██▀  ██▌     ,██▀

18: import "contracts/external/openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";

19: import "contracts/external/openzeppelin/contracts-upgradeable/token/ERC20/IERC20MetadataUpgradeable.sol";

20: import "contracts/external/openzeppelin/contracts-upgradeable/proxy/Initializable.sol";

21: import "contracts/external/openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";

22: import "contracts/external/openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";

23: import "contracts/external/openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";

117:     __rUSDY_init(blocklist, allowlist, sanctionsList, _usdy, guardian, _oracle);

227:     return (_sharesOf(_account) * oracle.getPrice()) / (1e18 * BPS_DENOMINATOR);

245:   function transfer(address _recipient, uint256 _amount) public returns (bool) {

635:       require(!_isSanctioned(msg.sender), "rUSDY: 'sender' address sanctioned");

File: contracts/usdy/rUSDYFactory.sol

7: ██  ██ ╒█▀'   ╙█▌ ╙█▌ ██     ▐██      ███  █████,  ██  ██▌    └██▌  ██▌     └██▌

8: ██ ▐█▌ ██      ╟█  █▌ ╟█     ██▌      ▐██  ██ └███ ██  ██▌     ╟██ j██       ╟██

9: ╟█  ██ ╙██    ▄█▀ ▐█▌ ██     ╙██      ██▌  ██   ╙████  ██▌    ▄██▀  ██▌     ,██▀

Low Issues

Issue
L-1Initializing state-variables in proxy-based upgradeable contracts
L-2Avoid transfer()/send() as reentrancy mitigations
L-3The critical parameters in initialize(...) are not set safely
L-4UNIFY RETURN CRITERIA
L-5REQUIRE MESSAGES ARE TOO SHORT AND UNCLEAR
L-6Timestamp Dependence
L-7Account existence check for low-level calls
L-8USE SAFETRANSFER INSTEAD OF TRANSFER

[L-1] Initializing state-variables in proxy-based upgradeable contracts

Description:

This should be done in initializer functions and not as part of the state variable declarations in which case they won’t be set.

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#avoid-initial-values-in-field-declarations

Proof Of Concept
File: contracts/usdy/rUSDY.sol

109:   function initialize(

File: contracts/usdy/rUSDYFactory.sol

90:     rUSDYProxied.initialize(

[L-2] Avoid transfer()/send() as reentrancy mitigations

Description:

Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes may break deployed contracts. Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://swcregistry.io/docs/SWC-134

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

324:     IRWALike(_token).transfer(owner(), balance);

File: contracts/usdy/rUSDY.sol

454:     usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);

680:     usdy.transfer(msg.sender, sharesAmount / BPS_DENOMINATOR);

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

[L-3] The critical parameters in initialize(...) are not set safely

Proof Of Concept
File: contracts/usdy/rUSDY.sol

109:   function initialize(

File: contracts/usdy/rUSDYFactory.sol

90:     rUSDYProxied.initialize(

[L-4] UNIFY RETURN CRITERIA

Description:

In files sometimes the name of the return variable is not defined and sometimes is, unifying the way of writing the code makes the code more uniform and readable.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

177:   function _checkThresholdMet(bytes32 txnHash) internal view returns (bool) {

361:   function getNumApproved(bytes32 txnHash) external view returns (uint256) {

File: contracts/bridge/SourceBridge.sol

162:   ) external payable override onlyOwner returns (bytes[] memory results) {

File: contracts/rwaOracles/IRWADynamicOracle.sol

20:   function getPrice() external view returns (uint256);

File: contracts/rwaOracles/RWADynamicOracle.sol

61:     returns (uint256 price, uint256 timestamp)

75:   function getPrice() public view whenNotPaused returns (uint256 price) {

110:   ) external view returns (uint256 price) {

265:   ) internal pure returns (uint256 price) {

282:   function roundUpTo8(uint256 value) internal pure returns (uint256) {

349:   ) internal pure returns (uint256 z) {

400:   function _rmul(uint256 x, uint256 y) internal pure returns (uint256 z) {

404:   function _mul(uint256 x, uint256 y) internal pure returns (uint256 z) {

File: contracts/usdy/rUSDY.sol

194:   function name() public pure returns (string memory) {

202:   function symbol() public pure returns (string memory) {

209:   function decimals() public pure returns (uint8) {

216:   function totalSupply() public view returns (uint256) {

226:   function balanceOf(address _account) public view returns (uint256) {

245:   function transfer(address _recipient, uint256 _amount) public returns (bool) {

259:   ) public view returns (uint256) {

276:   function approve(address _spender, uint256 _amount) public returns (bool) {

305:   ) public returns (bool) {

330:   ) public returns (bool) {

356:   ) public returns (bool) {

372:   function getTotalShares() public view returns (uint256) {

381:   function sharesOf(address _account) public view returns (uint256) {

390:   ) public view returns (uint256) {

397:   function getRUSDYByShares(uint256 _shares) public view returns (uint256) {

419:   ) public returns (uint256) {

500:   function _sharesOf(address _account) internal view returns (uint256) {

546:   ) internal whenNotPaused returns (uint256) {

578:   ) internal whenNotPaused returns (uint256) {

File: contracts/usdy/rUSDYFactory.sol

81:   ) external onlyGuardian returns (address, address, address) {

128:   ) external payable override onlyGuardian returns (bytes[] memory results) {

add {return x} if you want to return the updated value or else remove returns(uint) from the function(){} if no value you wanted to return

[L-5] REQUIRE MESSAGES ARE TOO SHORT AND UNCLEAR

Description:

The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory.

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

377:             revert(0, 0)

381:             revert(0, 0)

387:               revert(0, 0)

391:               revert(0, 0)

405:     require(y == 0 || (z = x * y) / y == x);

Error definitions should be added to the require block, not exceeding 32 bytes or we should use custom errors

[L-6] Timestamp Dependence

Description:

Contracts often need access to time values to perform certain types of functionality. Values such as block.timestamp, and block.number can give you a sense of the current time or a time delta, however, they are not safe to use for most purposes.

In the case of block.timestamp, developers often attempt to use it to trigger time-dependent events. As Ethereum is decentralized, nodes can synchronize time only to some degree. Moreover, malicious miners can alter the timestamp of their blocks, especially if they can gain advantages by doing so. However, miners cant set a timestamp smaller than the previous one (otherwise the block will be rejected), nor can they set the timestamp too far ahead in the future. Taking all of the above into consideration, developers cant rely on the preciseness of the provided timestamp.

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

64:     timestamp = block.timestamp;

79:       if (range.start <= block.timestamp) {

80:         if (range.end <= block.timestamp) {

83:           return derivePrice(range, block.timestamp);

[L-7] Account existence check for low-level calls

Description:

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling if needed.

https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-callsn

Proof Of Concept
File: contracts/bridge/SourceBridge.sol

165:       (bool success, bytes memory ret) = address(exCallData[i].target).call{

File: contracts/usdy/rUSDYFactory.sol

131:       (bool success, bytes memory ret) = address(exCallData[i].target).call{

In addition to the zero-address checks, add a check to verify that <address>.code.length > 0

[L-8] USE SAFETRANSFER INSTEAD OF TRANSFER

Description:

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

For example, Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)‘s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

324:     IRWALike(_token).transfer(owner(), balance);

File: contracts/usdy/rUSDY.sol

454:     usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);

680:     usdy.transfer(msg.sender, sharesAmount / BPS_DENOMINATOR);

Consider using safeTransfer/safeTransferFrom or require() consistently.

#0 - c4-pre-sort

2023-09-08T07:57:39Z

raymondfam marked the issue as sufficient quality report

#1 - c4-judge

2023-09-24T05:46:50Z

kirk-baird marked the issue as grade-a

#2 - ali2251

2023-09-25T13:34:02Z

NC-1 | ADD A TIMELOCK TO CRITICAL FUNCTIONS.
This will increase complexity and not needed for setting rate limits

NC-2 | ADD TO BLACKLIST FUNCTION no NFTS are used anywhere in the code so this is completely irrelevant

NC-3 | GENERATE PERFECT CODE HEADERS EVERY TIME this makes sense but doesnt add much value

NC-4 | SAME CONSTANT REDEFINED ELSEWHERE there is one constant defined in 2 places which is absolutely fine, hence no value add

NC-5 | IMPLEMENTATION CONTRACT MAY NOT BE INITIALIZED This is not true because initializers are disabled in the contructor

NC-6 | MARK VISIBILITY OF INITIALIZE(…) FUNCTIONS AS EXTERNAL doesnt matter because there are no child contracts

NC-7 | CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT doesnt make sense at all, there is no reason for a deterministic function to be immutable unless it can be changed in the constructor

NC-8 | LACK OF EVENT EMISSION AFTER CRITICAL INITIALIZE() FUNCTIONS its arguable whether it needs an event since no one uses it

NC-9 | FOR EXTENDED “USING-FOR” USAGE, USE THE LATEST PRAGMA VERSION its not required

NC-10 | NO SAME VALUE INPUT CONTROL doesnt make sense

NC-11 | Add parameter to event-emit NC-12 | SOLIDITY COMPILER OPTIMIZATIONS CAN BE PROBLEMATIC NC-13 | LINES ARE TOO LONG

as shown above, most of these issues does either not exist or are improvements that dont help security or readability

#3 - c4-sponsor

2023-09-25T13:34:10Z

ali2251 (sponsor) disputed

Findings Information

🌟 Selected for report: c3phas

Also found by: 0x11singh99, 0xhex, 0xta, Eurovickk, K42, MohammedRizwan, SAAJ, SAQ, SY_S, adriro, albahaca, castle_chain, jeffy, kaveyjoe, matrix_0wl, naman1778, petrichor, wahedtalash77, ybansal2403, zabihullahazadzoi

Awards

9.7506 USDC - $9.75

Labels

bug
G (Gas Optimization)
grade-b
sufficient quality report
G-20

External Links

Link to GitHub issue

Gas Optimizations

Issue
GAS-1ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED()
GAS-2Use assembly to check for address(0)
GAS-3Setting the constructor to payable
GAS-4DUPLICATED REQUIRE()/REVERT() CHECKS SHOULD BE REFACTORED TO A MODIFIER OR FUNCTION
GAS-5USE FUNCTION INSTEAD OF MODIFIERS
GAS-6CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT
GAS-7KECCAK256() SHOULD ONLY NEED TO BE CALLED ON A SPECIFIC STRING LITERAL ONCE
GAS-8MAKING CONSTANT VARIABLES PRIVATE WILL SAVE GAS DURING DEPLOYMENT
GAS-9Functions guaranteed to revert when called by normal users can be marked payable
GAS-10++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) = for-loop and while-loops
GAS-11The increment in for loop postcondition can be made unchecked
GAS-12Using private rather than public for constants, saves gas
GAS-13TERNARY OPERATION IS CHEAPER THAN IF-ELSE STATEMENT
GAS-14Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead
GAS-15USE BYTES32 INSTEAD OF STRING

[GAS-1] ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED()

Description:

Use abi.encodePacked() where possible to save gas

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

99:     if (chainToApprovedSender[srcChain] != keccak256(abi.encode(srcAddr))) {

238:     chainToApprovedSender[srcChain] = keccak256(abi.encode(srcContractAddress));

File: contracts/bridge/SourceBridge.sol

79:     bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);

[GAS-2] Use assembly to check for address(0)

Description:

Saves 6 gas per instance

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

113:     emit MessageReceived(srcChain, srcSender, amt, nonce);

File: contracts/usdy/rUSDY.sol

511:    * - `_sender` must hold at least `_sharesAmount` shares.

512:    * - the contract must not be paused.

536:    * @dev This doesn't increase the token total supply.

540:    * - `_recipient` cannot be the zero address.

560:     // as the result. This is equivalent to performing a send from each other token holder's

601:     return totalShares;

659:    * @dev The new oracle must comply with the `IPricerReader` interface

667:    * @notice Admin burn function to burn rUSDY tokens from any account

[GAS-3] Setting the constructor to payable

Description:

Saves ~13 gas per instance

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

55:   constructor(

File: contracts/bridge/SourceBridge.sol

40:   constructor(

File: contracts/rwaOracles/RWADynamicOracle.sol

30:   constructor(

File: contracts/usdy/rUSDY.sol

105:   constructor() {

File: contracts/usdy/rUSDYFactory.sol

51:   constructor(address _guardian) {

[GAS-4] DUPLICATED REQUIRE()/REVERT() CHECKS SHOULD BE REFACTORED TO A MODIFIER OR FUNCTION

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

377:             revert(0, 0)

381:             revert(0, 0)

387:               revert(0, 0)

391:               revert(0, 0)

File: contracts/usdy/rUSDY.sol

634:       require(!_isBlocked(msg.sender), "rUSDY: 'sender' address blocked");

635:       require(!_isSanctioned(msg.sender), "rUSDY: 'sender' address sanctioned");

644:       require(!_isBlocked(from), "rUSDY: 'from' address blocked");

645:       require(!_isSanctioned(from), "rUSDY: 'from' address sanctioned");

646:       require(_isAllowed(from), "rUSDY: 'from' address not on allowlist");

651:       require(!_isBlocked(to), "rUSDY: 'to' address blocked");

652:       require(!_isSanctioned(to), "rUSDY: 'to' address sanctioned");

653:       require(_isAllowed(to), "rUSDY: 'to' address not on allowlist");

[GAS-5] USE FUNCTION INSTEAD OF MODIFIERS

Proof Of Concept
File: contracts/usdy/rUSDYFactory.sol

154:   modifier onlyGuardian() {

[GAS-6] CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

27:   bytes32 public constant SETTER_ROLE = keccak256("SETTER_ROLE");

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

97:   bytes32 public constant USDY_MANAGER_ROLE = keccak256("ADMIN_ROLE");

98:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

100:   bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE");

[GAS-7] KECCAK256() SHOULD ONLY NEED TO BE CALLED ON A SPECIFIC STRING LITERAL ONCE

Description:

It should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once.

Proof Of Concept
File: contracts/rwaOracles/RWADynamicOracle.sol

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

[GAS-8] MAKING CONSTANT VARIABLES PRIVATE WILL SAVE GAS DURING DEPLOYMENT

Description:

When constants are marked public, extra getter functions are created, increasing the deployment cost. Marking these functions private will decrease gas cost. One can still read these variables through the source code. If they need to be accessed by an external contract, a separate single getter function can be used to return all constants as a tuple. There are four instances of public constants.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

48:   bytes32 public constant VERSION = "1.0";

File: contracts/bridge/SourceBridge.sol

27:   bytes32 public constant VERSION = "1.0";

File: contracts/rwaOracles/RWADynamicOracle.sol

23:   uint256 public constant DAY = 1 days;

27:   bytes32 public constant SETTER_ROLE = keccak256("SETTER_ROLE");

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

91:   uint256 public constant BPS_DENOMINATOR = 10_000;

97:   bytes32 public constant USDY_MANAGER_ROLE = keccak256("ADMIN_ROLE");

98:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

100:   bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE");

101:   bytes32 public constant LIST_CONFIGURER_ROLE =

File: contracts/usdy/rUSDYFactory.sol

44:   bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);

[GAS-9] Functions guaranteed to revert when called by normal users can be marked payable

Description:

If a function modifier such as onlyOwner/onlyX is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

210:   function addApprover(address approver) external onlyOwner {

220:   function removeApprover(address approver) external onlyOwner {

237:   ) external onlyOwner {

259:   ) external onlyOwner {

286:   function setMintLimit(uint256 mintLimit) external onlyOwner {

295:   function setMintLimitDuration(uint256 mintDuration) external onlyOwner {

304:   function pause() external onlyOwner {

313:   function unpause() external onlyOwner {

322:   function rescueTokens(address _token) external onlyOwner {

File: contracts/bridge/SourceBridge.sol

124:   ) external onlyOwner {

136:   function pause() external onlyOwner {

145:   function unpause() external onlyOwner {

162:   ) external payable override onlyOwner returns (bytes[] memory results) {

File: contracts/rwaOracles/RWADynamicOracle.sol

154:   ) external onlyRole(SETTER_ROLE) {

192:   ) external onlyRole(DEFAULT_ADMIN_ROLE) {

241:   function pauseOracle() external onlyRole(PAUSER_ROLE) {

248:   function unpauseOracle() external onlyRole(DEFAULT_ADMIN_ROLE) {

File: contracts/usdy/rUSDY.sol

127:   ) internal onlyInitializing {

138:   ) internal onlyInitializing {

662:   function setOracle(address _oracle) external onlyRole(USDY_MANAGER_ROLE) {

675:   ) external onlyRole(BURNER_ROLE) {

685:   function pause() external onlyRole(PAUSER_ROLE) {

689:   function unpause() external onlyRole(USDY_MANAGER_ROLE) {

700:   ) external override onlyRole(LIST_CONFIGURER_ROLE) {

711:   ) external override onlyRole(LIST_CONFIGURER_ROLE) {

722:   ) external override onlyRole(LIST_CONFIGURER_ROLE) {

File: contracts/usdy/rUSDYFactory.sol

81:   ) external onlyGuardian returns (address, address, address) {

128:   ) external payable override onlyGuardian returns (bytes[] memory results) {

154:   modifier onlyGuardian() {

[GAS-10] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) = for-loop and while-loops

Proof Of Concept
File: contracts/bridge/SourceBridge.sol

79:     bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);

[GAS-11] The increment in for loop postcondition can be made unchecked

Description:

This is only relevant if you are using the default solidity checked arithmetic. the for loop postcondition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks.One can manually do this.

Source

Proof Of Concept
File: contracts/bridge/SourceBridge.sol

79:     bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);

[GAS-12] Using private rather than public for constants, saves gas

Description:

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

48:   bytes32 public constant VERSION = "1.0";

File: contracts/bridge/SourceBridge.sol

27:   bytes32 public constant VERSION = "1.0";

File: contracts/rwaOracles/RWADynamicOracle.sol

23:   uint256 public constant DAY = 1 days;

27:   bytes32 public constant SETTER_ROLE = keccak256("SETTER_ROLE");

28:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File: contracts/usdy/rUSDY.sol

91:   uint256 public constant BPS_DENOMINATOR = 10_000;

97:   bytes32 public constant USDY_MANAGER_ROLE = keccak256("ADMIN_ROLE");

98:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

99:   bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

100:   bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE");

101:   bytes32 public constant LIST_CONFIGURER_ROLE =

File: contracts/usdy/rUSDYFactory.sol

44:   bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);

[GAS-13] TERNARY OPERATION IS CHEAPER THAN IF-ELSE STATEMENT

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

179:  if (t.numberOfApprovalsNeeded <= t.approvers.length) {
      return true;
    } else {
      return false;
    }

File: contracts/rwaOracles/RWADynamicOracle.sol

80:     if (range.end <= block.timestamp) {
          return derivePrice(range, range.end - 1);
        } else {
          return derivePrice(range, block.timestamp);
        }

132:    if (range.end <= blockTimeStamp) {
          return derivePrice(range, range.end - 1);
        } else {
          return derivePrice(range, blockTimeStamp);
        }

[GAS-14] Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

Description:

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed.

Proof Of Concept
File: contracts/usdy/rUSDY.sol

209:   function decimals() public pure returns (uint8) {

[GAS-15] USE BYTES32 INSTEAD OF STRING

Description:

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Proof Of Concept
File: contracts/bridge/DestinationBridge.sol

44:   mapping(string => bytes32) public chainToApprovedSender;

52:   mapping(string => Threshold[]) public chainToThresholds;

86:     string calldata srcChain,

87:     string calldata srcAddr,

131:     string memory srcChain

235:     string calldata srcChain,

236:     string calldata srcContractAddress

256:     string calldata srcChain,

405:   event ChainIdSupported(string srcChain, string approvedSource);

405:   event ChainIdSupported(string srcChain, string approvedSource);

414:   event ThresholdSet(string chain, uint256[] amounts, uint256[] numOfApprovers);

433:     string srcChain,

File: contracts/bridge/SourceBridge.sol

15:   mapping(string => string) public destChainToContractAddr;

15:   mapping(string => string) public destChainToContractAddr;

63:     string calldata destinationChain

66:     string memory destContract = destChainToContractAddr[destinationChain];

92:     string calldata destinationChain,

93:     string memory destContract,

122:     string memory destinationChain,

184:     string indexed destinationChain,

File: contracts/usdy/rUSDY.sol

194:   function name() public pure returns (string memory) {

202:   function symbol() public pure returns (string memory) {

File: contracts/usdy/rUSDYFactory.sol

150:     string name,

151:     string ticker

#0 - c4-pre-sort

2023-09-08T14:24:32Z

raymondfam marked the issue as sufficient quality report

#1 - c4-judge

2023-09-24T06:07:21Z

kirk-baird marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax Š 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter