QA Report

[code423n4]

Approve not compatible with Tether (USDT) implementation

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT or CVX)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

     // To change the approve amount you first have to reduce the addresses`
     //  allowance to zero by calling `approve(_spender, 0)` if it is not
     //  already 0 to mitigate the race condition described here:
     //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
     require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

     allowed[msg.sender][_spender] = _value;
     Approval(msg.sender, _spender, _value);
 }

The code as currently implemented does not handle these sorts of tokens properly, which would prevent USDT or CVX, from being used by this project.

If the method allPools return pools with duplicate underlyings, it will fault in the case of USDT or CVX, for example.

Affected source code:

• PoolMigrationZap.sol#L27

Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• RoleManager.sol#L32
• Authorization.sol#L10
• RoleManager.sol#L151
• RoleManager.sol#L157
• StakerVault.sol#L71
• BkdLocker.sol#L49-L50
• AddressProvider.sol#L50
• AddressProvider.sol#L56
• AddressProvider.sol#L65
• AddressProvider.sol#L70
• AddressProvider.sol#L81
• BkdToken.sol#L21
• FeeBurner.sol#L32
• VestedEscrowRevocable.sol#L43
• KeeperGauge.sol#L48
• VestedEscrow.sol#L52
• VestedEscrow.sol#L55
• AmmGauge.sol#L39

If you set any wrong address, without being a contract, an EOA for example, it cannot be changed again:

• Controller.sol#L36
• Minter.sol#L100
• InflationManager.sol#L61

Lack of int range checks:

• Minter.sol#L61-L69

Not max defined:

• Preparable.sol#L29

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• RoleManager.sol#L41-L48
• VestedEscrow.sol#L71

Avoid errors with transferFrom

The following methods take all the user's balance and send it through transferFrom, this call may fail, since transferFrom extracts the balance from the previously approved allowance, it is better to use the user's allowance in order to avoid the unnecessary failure when both amounts are not the same. It's better to use allowance instead of balanceOf.

Affected source code:

• FeeBurner.sol#L68-L70

Wrong logic around grantRole and revokeRole.

The RoleManager contract contains a few special roles (Roles.ADDRESS_PROVIDER, Roles.POOL_FACTORY, Roles.CONTROLLER, Roles.POOL and Roles.VAULT) that are not controlled in the grantRole and revokeRole methods.

Affected source code:

• RoleManager.sol#L37
• RoleManager.sol#L111

Centralization risks

Multiple initialization

The initialize method of the BkdLocker contract allows it to be started multiple times as long as the value startBoost=0 is set. Abuse these settings to his advantage.

Affected source code:

• BkdLocker.sol#L53-L63

Centralized minting

The minter address can mint arbitrary amount of tokens. If the private key of the owner or minter address is compromised, the attacker will be able to mint an unlimited amount of tokens, or burn from arbitrary addresses.

Affected source code:

• BkdToken.sol#L30

Controlled swapRouter

The FeeBurner contract sets the swapperRouter in the _addressProvider, so the owner can set any type of swapper, paths or pools, even malicious ones.
Since there is no slippage defined in the FeeBurner contract itself, it could be that a swapperRouter returns 0 WETH, and keeps the sent tokens.

Affected source code:

• FeeBurner.sol#L73

[GalloDaSballo]

Approve not compatible with Tether (USDT) implementation

Invalid as the approval happen once, from zero, to the max

Lack of checks

Valid per industry standard

## Lack of ACK during owner change
Personally disagree, but completely acceptable suggestion

Avoid errors with transferFrom

I believe this finding to be invalid in lack of acknowledging the broader system

Wrong logic around grantRole and revokeRole.

Idem

Multiple initialization

Dup of #136
Which I've yet to judge (may raise severity of this one)

Centralized minting

Observation is valid, but lacks nuance as the minter is a smart contract under scope

Controlled swapRouter

Agree with this finding and may raise severity as well as Duplicate of #113

[GalloDaSballo]

Overall most findings are pretty common from CodeArena, however the format is short and to the point, which I appreciate especially for the more basic findings

[GalloDaSballo]

Will raise
Multiple initialization
and
Controlled swapRouter

To 2 separate medium findings