The following require statement might need some refactoring to better accomplish its intended purpose. First off, it does not capture whether or not both staticAmount and dynamicAmount are zeroes at the same time.
require(
staticAmount == 0 || dynamicAmount == 0,
StaticATokenErrors.ONLY_ONE_AMOUNT_FORMAT_ALLOWED
);
Secondly, staticAmount and dynamicAmount have separately been hardcoded to 0 in the following two calling functions.
return _withdraw(msg.sender, recipient, amount, 0, toUnderlying); return _withdraw(msg.sender, recipient, 0, amount, toUnderlying);Here's a suggested fix:
require(
(staticAmount == 0 && dynamicAmount != 0) || (dynamicAmount == 0 && staticAmount != 0),
StaticATokenErrors.ONLY_ONE_AMOUNT_FORMAT_ALLOWED
);The plugin rewards are claimable via the following methods:
accRewardsPerTokenmultiplied by the pro-rate static share as adopted in StaticATokenLM- the delta of
accrued - claimedas adopted in CusdcV3Wrapper.sol - the delta of
balanceAfterClaimingRewards - _previousBalancefeatured by RewardableERC20.sol and inherited by other plugins like MorphoTokenisedDeposit.sol
While no loss of funds is involved here, all residual balances due to accidentally sent in, failure of transfers, etc will be forever stuck in the contract.
- uint48 delayUntilDefault; // {s} The number of seconds an oracle can mulfunction
+ uint48 delayUntilDefault; // {s} The number of seconds an oracle can malfunction- // In this case, the asset may recover, reachiving _whenDefault == NEVER.
+ // In this case, the asset may recover, reachieving _whenDefault == NEVER.- * @notice Updates rewards for senders and receiver in a transfer (not updating rewards for address(0))
+ * @notice Updates rewards for senders and receivers in a transfer (not updating rewards for address(0))- * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet unclaimed) rewards in RAY (rounded down).
+ * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet claimed) rewards in RAY (rounded down).- /// Takes `amount` fo cUSDCv3 from `src` and deposits to `dst` account in the wrapper.
+ /// Takes `amount` of cUSDCv3 from `src` and deposits to `dst` account in the wrapper.- // an immutable array so we do not have store the token feeds in the blockchain. This is
+ // an immutable array so we do not have to store the token feeds in the blockchain. This is- // I know this lots extremely verbose and quite silly, but it actually makes sense:
+ // I know this is extremely verbose and quite silly, but it actually makes sense:Although this will take a very long time to happen, the following function will default to true, i.e. DISABLED when block.timestamp exceeds type(uint48).max considering _whenDefault would at most be assigned NEVER, which is type(uint48).max.
function alreadyDefaulted() internal view returns (bool) {
return _whenDefault <= block.timestamp;
}The unit of pegPrice in EURFiatCollateral.tryPrice is target/ref but it is stated otherwise in the function NatSpec.
- /// @return pegPrice {UoA/ref}
+ /// @return pegPrice {target/ref}<= and >= have respectively been used in the if and else if clauses, making the = sign of the inequalities in the commented else clause erroneous.
uint48 delta = uint48(block.timestamp) - lastSave; // {s}
if (delta <= oracleTimeout) {
lotLow = savedLowPrice;
lotHigh = savedHighPrice;
} else if (delta >= oracleTimeout + priceTimeout) {
return (0, 0); // no price after full timeout
} else {
- // oracleTimeout <= delta <= oracleTimeout + priceTimeout
+ // oracleTimeout < delta < oracleTimeout + priceTimeout
// {1} = {s} / {s}
uint192 lotMultiplier = divuu(oracleTimeout + priceTimeout - delta, priceTimeout);
// {UoA/tok} = {UoA/tok} * {1}
lotLow = savedLowPrice.mul(lotMultiplier);
lotHigh = savedHighPrice.mul(lotMultiplier);
}The comment below is missing out the units pertaining to the division by one.
- // {qRewards} = {qRewards/share} * {qShare}
- // {qRewards} = {qRewards/share} * {qShare} / {qShare/share}
_accumuatedRewards += (delta * shares) / one;State variable names should be Camel-cased instead of fully capitalized (typically used on constants and immutables).
Here are some of the instances entailed:
ILendingPool public override LENDING_POOL;
IAaveIncentivesController public override INCENTIVES_CONTROLLER;
IERC20 public override ATOKEN;
IERC20 public override ASSET;
IERC20 public override REWARD_TOKEN;For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.
For example, the import instances below could be refactored conforming to the suggested standards as follows:
- import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
+ import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
- import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
- import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+ import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
- import "./RewardableERC20.sol";
+ import {RewardableERC20} from "./RewardableERC20.sol";It has lately been suggested that using enums instead of numbers for designated array indices makes the code logic more descriptive and readable.
Here are some of the instances entailed:
132: token0 = tokens[0];
133: token1 = tokens[1];
144: bool more = config.feeds[0].length > 0;
147: _t0feed0 = more ? config.feeds[0][0] : AggregatorV3Interface(address(0));
148: _t0timeout0 = more && config.oracleTimeouts[0].length > 0 ? config.oracleTimeouts[0][0] : 0;
149: _t0error0 = more && config.oracleErrors[0].length > 0 ? config.oracleErrors[0][0] : 0;Starting with Solidity version [0.8.8](https://docs.soliditylang.org/en/v0.8.20/contracts.html#function-overriding], using the override keyword when the function solely overrides an interface function, and the function doesn't exist in multiple base contracts, is unnecessary.
Here are some of the instances entailed:
https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/NonFiatCollateral.sol#L38-L42 https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/SelfReferentialCollateral.sol#L26-L30
function tryPrice()
external
view
override
returns ( 49: function _underlyingRefPerTok() internal view override returns (uint192) {
56: function claimRewards() external virtual override(Asset, IRewardable) {Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables, and more. Please visit the following link for further details:
https://docs.soliditylang.org/en/v0.8.16/natspec-format.html
It's generally incomplete throughout all codebases. Consider fully equipping all contracts with a complete set of NatSpec to better facilitate users/developers interacting with the protocol's smart contracts.
According to Solidity's Style Guide below:
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:
constructor, receive function (if exists), fallback function (if exists), external, public, internal, private
And, within a grouping, place the view and pure functions last.
Additionally, inside each contract, library or interface, use the following order:
type declarations, state variables, events, modifiers, functions
Consider adhering to the above guidelines for all contract instances entailed.
Passing zero as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter, asset amount, etc). Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
Here are some of the instances entailed:
return _withdraw(msg.sender, recipient, amount, 0, toUnderlying); return _withdraw(msg.sender, recipient, 0, amount, toUnderlying);The interfaces below should be defined in separate files, so that it's easier for future projects to import them, and to avoid duplication later on if they need to be used elsewhere in the project.
Here is one specific instance entailed that has been embedded in ATokenFiatCollateral.sol:
interface IStaticAToken is IERC20Metadata {