BalancerMaxis / ChildGaugeInjectorV2

1 stars 0 forks source link

QA: misleading comment in `@notice` in `changeDistributor` method #19

Closed petrovska-petro closed 1 month ago

petrovska-petro commented 1 month ago

Severity: unclear design intention

Context: if the intention indeed is to set the distrubutor into the owner, we could omit a parameter and simply hardcoded the owner

Recommendation: code suggestion below

/**
* @notice Set distributor from the injector back to the owner. ---> owner???
* ...
*/
function changeDistributor(
    address gauge,
    address reward_token,
-    // address distributor 
) external onlyOwner {
    IChildChainGauge(gauge).set_reward_distributor(
        reward_token,
+        owner() ---> since it's defined as private needs to be access via getter
    );
}

If the design is open to any "distributor", then it is advisable to update the Natspec comments accordangly.

Review stage

Balancer Maxis:

Onchainification Labs:

Tritium-VLK commented 1 month ago

It was annoying to change back to owner, and then assign elsewhere. The V1 injector tried to prevent stupidity by only allowing sweeps/changes/etc back to the owner. The new version realizes that this is just annoying, and at best opens up functionality to allow the owner to do admin operations "to" any address.

Will correct the docs here. Let me know if you see this elsewhere.

pretty sure the contract as it stands exposes owner() as a read function. Take a look at the deployed version: https://polygonscan.com/address/0xF7D4E035182825EcCECeC489f08F349Ab214Cf82#readContract

Think this is included in Ownable2Step.sol.

petrovska-petro commented 1 month ago

natspec are being tackled in https://github.com/BalancerMaxis/ChildGaugeInjectorV2/pull/28, pendant of review

petrovska-petro commented 1 month ago

once #28 is merged, this issue can be closed ✅