Backd Tokenomics contest - StyxRave's results

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/swappers/SwapperRouter.sol#L280 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/swappers/SwapperRouter.sol#L140

Vulnerability details

Using payable(address).transfer is deprecated in favor of using .call{value:...}("") as the correct way of sending ETH. Using transfer or send will make transactions fail when the address corresponds to a contract that does not implement a payable function using less than 2300 gas (multisigs and proxied contracts). Implementations of this in SwapperRouter.sol will prevent such contracts from being able to correctly interact with the protocol.

Redundant greater than 0 checks for uint When executing pool weight changes in InflationManager.sol (_executeKeeperPoolWeight, _executeLpPoolWeight, _executeAmmTokenWeight), corresponding total pool weights x are unsigned int. As they can't only be x >= 0, the expression x>0 ? x : 0 does nothing.

Sensible changes, such as adding a Governor should be a two-step process addGovernance directly grants Roles.GOVERNANCE to the newGovernor. It should be a two-step process (e.g. setGovernance-acceptGovernance) to ensure the input address is correct. Whilst renounceGovernance checks that there is at least one Governor left, in a single-Governor scenario, back to back execution of addGovernance with an incorrect address followed by renounceGovernance will render the protocol ungovernable. Additionally, as there is no way to revoke this role, granting governance to an address means that address will have complete control over the protocol forever after.

Requirement uses external call to user-controlled address in PoolMigrationZap.sol - migrate require(oldPool_.getWithdrawalFee(msg.sender, lpTokenAmount_) == 0, "withdrawal fee not 0"); makes an external call to oldPoolAddress_, which is a user-controlled address. It could be a malicious contract, thus reporting any result needed to bypass the guard.

Erring on the safe side for reentrancy in PoolMigrationZap.sol - migrate While I did not manage to exploit it in any way - I'd recommend against not including a nonReentrant guard in this function. oldPoolAddress_ could be a malicious contract thus enabling multiple calls to migrate spanning from the external calls to oldPool or lpToken_.

Recommended missing events Modifying key protocol variables or triggering key events should always emit events accordingly to ensure transparency and proper traceability.

• Minter.sol: setToken, startInflation, executeInflationRateUpdate
• BkdLocker.sol: migrate
• Controller.sol: setInflationManager
• AmmGauge.sol: kill
• KeeperGauge.sol: kill, advanceEpoch, reportFees, poolCheckpoint
• InflationManager.sol: deactivateWeightBasedKeeperDistribution, checkpointAllGauges
• VestedEscrow.sol: setAdmin, setFundAdmin, initializeUnallocatedSupply

• PoolMigrationZap.sol - Use UncheckedMath.uncheckedInc instead of ++i in loop

• FeeBurner.sol - Use calldata instead of memory for read-only function argument array tokens_.