QA Report

[code423n4]

Low severity findings

During my audit of this project I found 5 low priority findings in the Gravity.sol contract that I think are worth mentioning:

1. Any whitelisted address can remove all other whitelisted addresses. This is in the manageWhitelist method here. This pattern doesn't make sense to me since you only need 1 bad actor in the whitelist for this situation to occur. The offending address can then frontrun any attempts from the admin to remove them from the whitelist by adding new addresses to the whitelist. I decided not to mark this as medium severity since I can't see the whitelist being used anywhere yet. I would update this method to only be callable by the admin.

2. The comment strings explaining submitBatch and updateValset say "anyone can call this function", but in reality the msg.sender has to be an orchestrator (i.e. one of the existing validators in the valset)

3. makeCheckpoint has the following comment string:

// The format of the checkpoint is:
// h(gravityId, "checkpoint", valsetNonce, validators[], powers[])

This should be updated to:

// h(gravityId, "checkpoint", valsetNonce, validators[], powers[], rewardAmount, rewardToken)

4. There is no check in checkValidatorSignatures to confirm whether or not the array of validator powers is decreasing. The validators should be passed in as arguments in decreasing power order to optimise gas. If you wanted to enforce this you could add a check as you loop through the array to verify that the current power is less than the previous power (in the array). This might use more gas than the arrays being out of order though, depending on the length of the arrays.

5. Test fixtures are still there and should be removed (seems obvious but easily forgotten)

[albertchon]

On point 2, actually anyone can call submitBatch and updateValset