Anchor contest - robee's results

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: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/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

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In CrossAnchorBridge.sol,rearranging the storage fields can optimize to: 3 slots from: 4 slots. The new order of types (you choose the actual variables):

    1. bytes32
    2. address
    3. uint32
    4. uint32
    5. uint16
    6. uint8
    7. uint8
    8. address
    9. uint8
    10. uint8
    11. uint8
    12. uint8
    13. uint8
    14. uint8
    15. uint8
    16. uint8
    17. uint8
    18. uint8

Title: uint8 index Severity: GAS

Due to how the EVM natively works on 256 numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type. See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage We recommend to use uint256 for the index in every for loop instead using uint8:

    CrossAnchorBridge.sol, uint8 i, 149

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    CrossAnchorBridge.sol, initialize

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    CrossAnchorBridge.sol, handleToken
    CrossAnchorBridge.sol, encodeAddress
    CrossAnchorBridge.sol, handleStableToken
    CrossAnchorBridge.sol, _authorizeUpgrade

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    CrossAnchorBridge.sol, 149

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    CrossAnchorBridge.processTokenTransferInstruction (encodedIncomingTokenTransferInfo)
    CrossAnchorBridge.processTokenTransferInstruction (encodedTokenTransfer)

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    CrossAnchorBridge.sol, OP_CODE_CLAIM_REWARDS
    CrossAnchorBridge.sol, FLAG_NO_ASSC_TRANSFER

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: CrossAnchorBridge.sol, i, 149

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    CrossAnchorBridge.sol, _collateralTokens, 149

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    CrossAnchorBridge.sol, encodeAddress, { return bytes32(uint256(uint160(addr))); }