KYC Whitelist

In the _lidoEthToBrktETH function the return value from the submit function is used as the stEth amount gained from the submit call.

However the return value represents the amount of stEth shares, not the amount of stEth which were gained from the submit call.

As a result a significant portion of stEth will be left in the router contract and the user will not receive bracketEth for this amount.

Use the difference between the stEth balance before and after calling the submit function to wrap in the wstEth contract.

Bracket Team: The issue was resolved in commit c23fed5.

The KYCWhitelist contract cannot be successfully deployed because the initializing functions used in the constructor use an onlyInitializing modifier.

Refactor the KYCWhitelist contract so that the constructor is the initializer or make a separate initializer function.

Bracket Team: The issue was resolved in commit 7b70cb2.

The STETH address in the Config contract is incorrect as it points to the STETH implementation contract (0x17144556fd3424EDC8Fc8A4C940B2D04936d17eb) rather than the correct proxy contract (0xae7ab96520de3a18e5e111b5eaab095312d7fe84).

Update the STETH address to be the correct proxy address.

Bracket Team: The issue was resolved in commit ca93c8a.

In the _update function there is no validation that the from address is whitelisted on transfer. This allows unwhitelisted addresses to transfer to whitelisted addresses which may be unexpected.

Consider adding validation that the from address is also whitelisted in the _update function.

Bracket Team: The issue was resolved in commit 70793e1.

The _update function performs _clearDeposit in all cases, even when the _update invocation is for a mint or burn call.

There are no significant issues from this as the _clearDeposit call will always early return for minting and burning updates.

However to avoid any hidden introduction of issues in the future and to optimize gas expenditure, this unexpected execution should be avoided.

Consider only invoking the _clearDeposit function when the from address and to address are both nonzero, indicating that the update action is a legitimate transfer.

Bracket Team: The issue was resolved in commit 6717a3d.

The wethToBrktETH function includes a payable modifier yet does not handle msg.value.

Remove the payable modifier from the wethToBrktETH function.

Bracket Team: The issue was resolved in commit 566e943.

The ETHToBrktETH event emission in the ethToBrktETH and wethToBrktETH functions emits the minimum acceptable brktEth amount rather than the actual minted brktEth amount from the action. This may be misleading for consumers of the ETHToBrktETH event.

Consider if the actual minted brktEth amount should be emitted in the ETHToBrktETH event.

Bracket Team: The issue was resolved in commit b0f4119.

The updateNav function updates the vanityNav but does not emit a VanityNavUpdated event. This may mislead indexers and consumers of the VanityNavUpdated event if they are not aware of this behavior.

Consider if this behavior is intended, if not, consider emitting the VanityNavUpdated event in the updateNav function.

Bracket Team: Acknowledged.

In the lidoLimit function the isStakingPaused function is queried on the steth contract. If staking is paused the result of the limit is 0.

However this logic is already included in the lido underlying getCurrentStakeLimit function and therefore is unnecessary in the lidoLimit function.

Consider removing the unnecessary isStakingPaused logic in the lidoLimit function.

Bracket Team: The issue was resolved in commit b677547.

The RocketDepositPool contract is hardcoded as a constant in the BrktEthRouter contract. However the latest RocketDepositPool is allowed to change in the RocketStorage contract.

Be aware of this possibility and consider adding a setter function for the RocketDepositPool contract in case it were to be updated by Rocket Pool.

Bracket Team: Acknowledged.