Initial implementation

[oddaf]

No description provided.

[amusingaxl]

I left a few comments.

There's also another issue that doesn't show in the PR:
https://github.com/dewiz-xyz/rates-conv/blob/master/foundry.toml#L8-L9

Do we want optimizations enabled? If so, why are we optimizing for 1 run? Is it to save on contract deployment size vs runtime cost?
Would it be possible to NOT have this contract optimized? Or if it's optimized, maybe use a higher value for runs?

Other than that, here's the reviewer checklist:

Code Review: 2025-02-17 rates-conv

Preparation and Context

1. Understand Requirements
   • Review related documentation, user stories, or issue tickets, and Technical Requirements Doc.
   • Ensure a clear understanding of the contract's purpose and functionality.
2. Assumptions and Constraints
   • Verify that these assumptions hold true under all expected and edge case conditions. Check assumptions in the Technical Requirement Doc

Contract Design

• Ensure the contract is modular and logically organized.
• Verify proper separation of concerns and encapsulation.
  ✅ The contract provides a clear separation of concerns (only conversion of rates) and is well-organized.
• Challenge the overall design and logic of the contract.
• Ensure that the design aligns with security best practices and minimizes attack surfaces.

Security Considerations

• Reentrancy: Check for vulnerabilities, ensure use of ReentrancyGuard if needed.
  ✅ N/A: the contract is immutable and read-only
• Integer Overflow/Underflow: Ensure usage of SafeMath library or built-in overflow checks.
  ✅ Assembly code has the proper overflow checks
  ✅ Solidity code uses the default safe math from 0.8.x
• Access Control: Verify correct use of access control modifiers (e.g., onlyOwner).
  ✅ N/A: the contract is immutable and permissionless
• Front-running: Assess susceptibility to front-running attacks and implement mitigation strategies.
• Randomness: Ensure secure generation of random numbers.
• Timestamp Dependence: Avoid reliance on block.timestamp for critical logic.
• External Calls: Minimize and carefully manage external contract calls.
• Denial of Service: Prevent potential DoS attacks, especially in payable functions.
• Further information https://swcregistry.io/, https://dacian.me/defi-slippage-attacks, https://www.rareskills.io/post/smart-contract-security, https://solodit.xyz/

General checks

• Visibility Specifiers: Ensure correct use of public, private, internal, and external.
  ⚠️ rtob should be external rather than public
  ⚠️ btor should be external rather than public
• Modifiers: Review custom and built-in modifiers for security and correctness.
• Pure and View: Use pure and view appropriately to save gas and convey intent.
• Fallback Functions: Implement securely, if used. Ensure correct handling of ether and gas limits.
• Check for potential gas limit issues with logging too many events.
• Verify that dependencies are pinned to specific, audited versions.
• Minimize storage writes and reads.
• Evaluate loop usage and optimize for gas efficiency.
• The interface should match fully the onchain ones.

Documentation

• Document functions and state variables using NatSpec comments.
• Provide an overview of the contract’s purpose and functionality.
  ❌ Missing
• Add custom headers into the codebase with Author and Reviewers names:

  /// @custom:authors: []
  /// @custom:reviewers: []
  /// @custom:auditors: []
  /// @custom:bounties: []
  

Testing and Verification

• Ensure comprehensive unit tests cover all scenarios and edge cases.
• Use Fuzzing in tests (1000 rounds)
• Using modifier & internal fn in test to improve readability/reusability of tests
• Fuzzing of storage vars and external call methods
• Use invariant test if applicable (Use the technical doc where invariants should be defined)
• Use formal vefication if applicable (Use the technical doc where invariants should be defined)
• Use static analysis tools (e.g., MythX, Slither) to detect common issues.

Adversarial Thinking

1. Hack the Code

   • Think like an attacker and attempt to exploit vulnerabilities.
   • Challenge assumptions and attempt to break the contract under different conditions.

2. Unpredictable System States

   • Consider how the contract behaves under unpredictable states and stress conditions.
   • Test for scenarios that might lead to unexpected behaviors.

3. List of Attack Vectors

   • Donation Attacks: Review and protect against unintended ether donations.
   • Flash Loans: Consider and mitigate risks from flash loan attacks.
   • Oracle Manipulation: Ensure secure and reliable use of oracles to prevent manipulation.
   • Gas Limit Exploits: Protect against attacks that exploit gas limits to disrupt contract functionality.
   • Phishing and Social Engineering: Ensure contract interfaces are designed to minimize phishing risks.

Code Review Checklist

General Review Approach

• Understand the context
  • Construct a mental model of the contract’s expected behavior.
  • Compare the actual architecture to your mental model.
  • Create a threat model and list high-level attack vectors.
  • Focus on functions handling value exchange (transfer, send, call, delegatecall, selfdestruct).
  • Review external contract interactions and validate assumptions.
  • Perform a line-by-line code review.
  • Review from the perspective of every actor in the threat model.
  • Evaluate test coverage and focus on untested areas.

Variables

• Visibility and Mutability
  • Can it be internal?
  • Can it be constant?
  • Can it be immutable?
  • Is its visibility set?
  • Document purpose and other important information using NatSpec.
• Storage Efficiency
  • Can it be packed with adjacent storage variables?
  • Can it be packed in a struct with more than one other variable?
  • Use full 256-bit types unless packing with other variables.
  • If public array, is there a function to return the full array?
  • Use private only to prevent child contracts from accessing the variable, prefer internal for flexibility.

Structs

• Necessity and Documentation
  • Is a struct necessary, or can variables be packed in storage?
  • Are fields packed together if possible?
  • Document purpose and all fields using NatSpec.

Functions

• Visibility and Functionality
  • Can it be external?
    • btor ❌
    • rtob ❌
  • Should it be internal?
    • _rpow ✅
  • Should it be payable?
  • Can it be combined with another similar function?
• Patterns and Security
  • Check for front-running possibilities, e.g., approve function.
  • Ensure return values are always assigned.
  • Test invariants about the state before function execution.
  • Test invariants about return values and state changes after function execution.
  • Document all arguments, return values, and side effects using NatSpec.
    ❌ Found some issues detailed in the review
  • If operating on another user, don’t assume msg.sender is the user.

Modifiers

• Safety and Documentation
  • Avoid storage updates (except in reentrancy lock).
  • Avoid external calls.
  • Document purpose and information using NatSpec.

Code

• ~~Do not mutate state before doing external calls to unknown contract ~~
• Safe Math and Efficiency
  • Use SafeMath or 0.8 checked math
    • unchecked blocks MUST contain a justification
  • Minimize redundant storage reads.
  • Avoid unbounded loops/arrays causing DoS
• Best Practices
  • Use block.timestamp only for long intervals
  • Avoid delegatecall especially to external contracts
  • Avoid updating array length while iterating.
  • Use abi.encode() instead of abi.encodePacked() with dynamic types
  • Don’t assume a specific ETH balance
  • Remember private data isn't private
  • Don’t mutate function parameters.
  • Compare costs of calculating vs storing values.
  • Multiply before dividing unless multiplication could overflow.
  • Replace magic numbers with constants.
  • Consider DoS if recipient’s fallback function reverts.
  • Safely check return values of ERC20 transfers.
  • Avoid msg.value in loops.
  • Avoid msg.value if recursive delegatecalls are possible.
  • Don’t rely on msg.sender in your logic if your contract can be called as delegate call.
  • Try to avoid using send funds logic use pull funds logic instead(see next check)
  • Try to avoid using payable.transfer(), use payable.call{value: }() (be aware of reentrancy) or payable.send() instead depending on use case
  • Use unchecked blocks where safe, and document reasons with gas estimates.
  • Use parentheses to emphasize arithmetic operator precedence.
  • Document precision loss in arithmetic operations and ensure it benefits the right actors.
• Go over the solodit.xyz/checklist
  ℹ️ Most checks don't apply, the ones who do are already covered in this checklist (i.e: SOL-Basics-Function-7)

[oddaf]

If so, why are we optimizing for 1 run? Is it to save on contract deployment size vs runtime cost?

Yes, lower runs optimize for deployment, a larger number of runs for execution. I was fighting deployment size at one point (to try to fit rates up to 60% within contract size limit), this is why 1 runs.

Now we can remove it, deployment size and gas cost are quite close, but it does have a large impact on read gas cost (~600 gas difference, from 3767 top 3123 (or 3111 with 200 runs). What do you think @amusingaxl ?

Edit: changed it to 200 runs as it is more conventional, and it results in the best read price (deployment difference from using 1 run is negligible and we're not fighting the size limit at 5k BPS

[amusingaxl]

If so, why are we optimizing for 1 run? Is it to save on contract deployment size vs runtime cost?

Yes, lower runs optimize for deployment, a larger number of runs for execution. I was fighting deployment size at one point (to try to fit rates up to 60% within contract size limit), this is why 1 runs.

Now we can remove it, deployment size and gas cost are quite close, but it does have a large impact on read gas cost (~600 gas difference, from 3767 top 3123 (or 3111 with 200 runs). What do you think @amusingaxl ?

Edit: changed it to 200 runs as it is more conventional, and it results in the best read price (deployment difference from using 1 run is negligible and we're not fighting the size limit at 5k BPS

What if we set runs ridiculously high?

I'm using the gas metering script. With 200 runs, I get:

Gas used: 28090985

== Logs ==
  Deploy:  27980081
  Turn bps 0 : 1000
  Turn bps 123 : 1000
  Turn bps 246 : 1000
  Turn bps 369 : 1000
  Turn bps 492 : 1000
  Turn bps 615 : 1000
  Turn bps 738 : 1000
  Turn bps 861 : 1000
  Turn bps 984 : 1000
  Turn bps 1107 : 1000
  Turn bps 1230 : 1000
  Turn bps 1353 : 1000
  Turn bps 1476 : 1000
  Turn bps 1599 : 1000
  Turn bps 1722 : 1000
  Turn bps 1845 : 1000
  Turn bps 1968 : 1000
  Turn bps 2091 : 1000
  Turn bps 2214 : 1000
  Turn bps 2337 : 1000
  Turn bps 2460 : 1000
  Turn bps 2583 : 1000
  Turn bps 2706 : 1000
  Turn bps 2829 : 1000
  Turn bps 2952 : 1000
  Turn bps 3075 : 1000
  Turn bps 3198 : 1000
  Turn bps 3321 : 1000
  Turn bps 3444 : 1000
  Turn bps 3567 : 1000
  Turn bps 3690 : 1000
  Turn bps 3813 : 1000
  Turn bps 3936 : 1000
  Turn bps 4059 : 1000
  Turn bps 4182 : 1000
  Turn bps 4305 : 1000
  Turn bps 4428 : 1000
  Turn bps 4551 : 1000
  Turn bps 4674 : 1000
  Turn bps 4797 : 1000
  Turn bps 4920 : 1000

With 2_000_000 runs, I get:

Gas used: 28099801

== Logs ==
  Deploy:  27991504
  Turn bps 0 : 964
  Turn bps 123 : 964
  Turn bps 246 : 964
  Turn bps 369 : 964
  Turn bps 492 : 964
  Turn bps 615 : 964
  Turn bps 738 : 964
  Turn bps 861 : 964
  Turn bps 984 : 964
  Turn bps 1107 : 964
  Turn bps 1230 : 964
  Turn bps 1353 : 964
  Turn bps 1476 : 964
  Turn bps 1599 : 964
  Turn bps 1722 : 964
  Turn bps 1845 : 964
  Turn bps 1968 : 964
  Turn bps 2091 : 964
  Turn bps 2214 : 964
  Turn bps 2337 : 964
  Turn bps 2460 : 964
  Turn bps 2583 : 964
  Turn bps 2706 : 964
  Turn bps 2829 : 964
  Turn bps 2952 : 964
  Turn bps 3075 : 964
  Turn bps 3198 : 964
  Turn bps 3321 : 964
  Turn bps 3444 : 964
  Turn bps 3567 : 964
  Turn bps 3690 : 964
  Turn bps 3813 : 964
  Turn bps 3936 : 964
  Turn bps 4059 : 964
  Turn bps 4182 : 964
  Turn bps 4305 : 964
  Turn bps 4428 : 964
  Turn bps 4551 : 964
  Turn bps 4674 : 964
  Turn bps 4797 : 964
  Turn bps 4920 : 964

Not sure how accurate those are though.

[oddaf]

What if we set runs ridiculously high?

We can do it.

These results are accurate, the difference will be exactly that. Only thing that need to be noted is these are prices for reading from a warm slot, so for cold slots the cost is the listed value + 2000.

[amusingaxl]

What if we set runs ridiculously high?

We can do it.

These results are accurate, the difference will be exactly that. Only thing that need to be noted is these are prices for reading from a warm slot, so for cold slots the cost is the listed value + 2000.

That was what I was wondering.
Why are the slots warm? Because the contract was deployed in the same "transaction"?

[oddaf]

Why are the slots warm? Because the contract was deployed in the same "transaction"?

It's due to running it within a script. On tests, the constructor and setUp() functions are run in different transactions, so we get the cold slot gas cost. In the script I tried moving around the contract deployment but no luck, storage positions are always warm, I think it's the way the script is ran in the VM (it runs as if it was a large tx including everything).

Only way to get cold slot gas cost would be pointing the script to a testnet and running it with --broadcast (we'd need to edit it and include startBroadcast/stopBroadcast, runner will need to setup a key and fund it with testnet ETH).

[amusingaxl]

Only way to get cold slot gas cost would be pointing the script to a testnet and running it with --broadcast (we'd need to edit it and include startBroadcast/stopBroadcast, runner will need to setup a key and fund it with testnet ETH).

No need, let's keep it as it is.
Doing a final round of reviews now.

[amusingaxl]

Found 2 additional nits.

[amusingaxl]

LGTM