QA Report #263
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Comments
code423n4
added
bug
|
great linking, great format! |
|
1 is invalid all of these seem non-critical except for 15, which is acknowledged and 3/12, which are valid |
This was referenced Aug 12, 2022
|
I would tend to agree that all of these are valid except for:
Even if we assert that this address is a contract, the issue whereby the owner role can force |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Overview
Table of Contents
_sponsorVaultis a contractal,feeandadminFeecannot be set the their maximum valueabi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256()__gap[50]storage variable to allow for new storage variables in later versionsinitialize()functions are front-runnable in the solutionconstantinstead of duplicating the same stringconstantinsteadnonReentrantmodifiershould occur before all other modifiers>= 0.8: SafeMathreturnstatement when the function defines a named return variable, is redundantLow Risk Issues
1. Check that
_sponsorVaultis a contract_sponsorVaultbeing an EOA instead of a contract can cause issues. Consider doing just like in setExecutor():File: BridgeFacet.sol 250: function setSponsorVault(address _sponsorVault) external onlyOwner { 251: address old = address(s.sponsorVault); - 252: if (old == _sponsorVault) revert BridgeFacet__setSponsorVault_invalidSponsorVault(); + 252: if (old == _sponsorVault || !Address.isContract(_sponsorVault)) revert BridgeFacet__setSponsorVault_invalidSponsorVault(); 253: 254: s.sponsorVault = ISponsorVault(_sponsorVault); 255: emit SponsorVaultUpdated(old, _sponsorVault, msg.sender); 256: }2.
al,feeandadminFeecannot be set the their maximum valueConsider replacing
<with<=here:3. Add constructor initializers
As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run
initializeon an implementation contract, by adding an empty constructor with theinitializermodifier. So the implementation contract gets initialized automatically upon deployment.”Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”
Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:
4. Unsafe casting may overflow
SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.
Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:
5. Uninitialized Upgradeable contract
Similar issue in the past: here
Upgradeable dependencies should be initialized.
While not causing any harm at the moment, suppose OZ someday decide to upgrade this contract and the sponsor uses the new version: it is possible that the contract will not work anymore.
Consider calling the
init()here:File: PromiseRouter.sol 23: contract PromiseRouter is Version, Router, ReentrancyGuardUpgradeable { ... 146: function initialize(address _xAppConnectionManager) public initializer { 147: __XAppConnectionClient_initialize(_xAppConnectionManager); + 147: __ReentrancyGuard_init(); //@audit ReentrancyGuardUpgradeable 148: }This is already applied L72 in
StableSwap.sol(but was forgotten inPromiseRouter.sol):6. Deprecated safeApprove() function
Deprecated
Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20
safeApprove()function has been deprecated, as seen in the comments of the OpenZeppelin code.As recommended by the OpenZeppelin comment, consider replacing
safeApprove()withsafeIncreaseAllowance()orsafeDecreaseAllowance()instead:7. Missing address(0) checks
Consider adding an
address(0)check for immutable variables:29: address private immutable connext; ... 47: constructor(address _connext) { + 48: require(_connext != address(0)); 48: connext = _connext; 49: }8. Misleading comment
Issue with comment
I suspect a copy-paste error here:
9. Add a timelock to critical functions
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).
Consider adding a timelock to:
10.
abi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256()Similar issue in the past: here
Use
abi.encode()instead which will pad items to 32 bytes, which will prevent hash collisions (e.g.abi.encodePacked(0x123,0x456)=>0x123456=>abi.encodePacked(0x1,0x23456), butabi.encode(0x123,0x456)=>0x0...1230...456). If there is only one argument toabi.encodePacked()it can often be cast tobytes()orbytes32()instead.11. Upgradeable contract is missing a
__gap[50]storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future:
Those contracts do apply the recommendation though:
12. All
initialize()functions are front-runnable in the solutionConsider adding some access control to them or deploying atomically or using constructor
initializer:13. Use the same revert string for consistency when testing the same condition
Issue with comment
Consider only using the shorter string:
14. Use a
constantinstead of duplicating the same stringIssue with comment
15. Use a 2-step ownership transfer pattern
Contracts inheriting from OpenZeppelin's libraries have the default
transferOwnership()function (a one-step process). It's possible that theonlyOwnerrole mistakenly transfers ownership to a wrong address, resulting in a loss of theonlyOwnerrole.Consider overriding the default
transferOwnership()function to first nominate an address as thependingOwnerand implementing anacceptOwnership()function which is called by thependingOwnerto confirm the transfer.16. A magic number should be documented and explained. Use a
constantinsteadSimilar issue in the past: here
Consider using
constantvariables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).17. Lack of event emission for operation changing the state
Non-Critical Issues
1. It's better to emit after all processing is done
2. The
nonReentrantmodifiershould occur before all other modifiersThis is a best-practice to protect against re-entrancy in other modifiers
3. Typos
4. Deprecated library used for Solidity
>= 0.8: SafeMath5. Open TODOS
Consider resolving the TODOs before deploying.
6. Adding a
returnstatement when the function defines a named return variable, is redundantWhile not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.
Affected code:
7. The pragmas used are not the same everywhere
Consider using only 1 version. Here's an example of the different pragmas:
8. Non-library/interface files should use fixed compiler versions, not floating ones
9. Missing NatSpec
The text was updated successfully, but these errors were encountered: