Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #1

Open
code423n4 opened this issue Feb 24, 2022 · 0 comments
Open

QA Report #1

code423n4 opened this issue Feb 24, 2022 · 0 comments
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
Copy link
Contributor

Title: Init frontrun
Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself.
(details credit to: code-423n4/2021-09-sushimiso-findings#64)
The vulnerable initialization functions in the codebase are:

    CrossAnchorBridge.sol, initialize, 135

Title: Named return issue
Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.

    CrossAnchorBridge.sol, encodeAddress

Title: safeApprove of openZeppelin is deprecated
Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base

Deprecated safeApprove in CrossAnchorBridge.sol line 189: SafeERC20.safeApprove(IERC20(token), WORMHOLE_TOKEN_BRIDGE, amount);

Title: Not verified input
Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    CrossAnchorBridge.sol.handleStableToken token
    CrossAnchorBridge.sol.lockCollateral token
    CrossAnchorBridge.sol.initialize _wormholeCoreBridge
    CrossAnchorBridge.sol.redeemStable token
    CrossAnchorBridge.sol.encodeAddress addr
    CrossAnchorBridge.sol.depositStable token
    CrossAnchorBridge.sol.initialize _wormholeTokenBridge
    CrossAnchorBridge.sol.repayStable token
    CrossAnchorBridge.sol.handleToken token
    CrossAnchorBridge.sol.initialize _aust
    CrossAnchorBridge.sol.initialize _wust
@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 24, 2022
code423n4 added a commit that referenced this issue Feb 24, 2022
@code423n4 code423n4 mentioned this issue Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

No branches or pull requests

1 participant