Backd Tokenomics contest - 0x1f8b's results

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

Outdated compiler

The pragma version used is pragma solidity 0.8.10; but recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• Errors.sol#L7-L111
• Minter.sol#L152

Delete optimization

Use delete instead of set to default value (false or 0)

Affected source code:

• RoleManager.sol#L158
• Preparable.sol#L87-L88
• Preparable.sol#L99-L100
• Preparable.sol#L112
• Preparable.sol#L142-L143
• Preparable.sol#L150-L151
• LpGauge.sol#L60
• AmmGauge.sol#L64

unckecked keyword

It's possible to save gas using the unckecked keyword around the i variable. This will avoid the required checks to ensure that the variable won't overflow, and in the case of this for loop, is almost impossible:

Reference:

• https://docs.soliditylang.org/en/v0.8.0/control-structures.html#checked-or-unchecked-arithmetic

Affected source code:

• PoolMigrationZap.sol#L22

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2. I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--. Also, in some of the points mentioned below, it would be necessary to add unckecked since said variable is impossible to overflow.

Affected source code:

• BkdLocker.sol#L140
• RoleManager.sol#L82
• RewardHandler.sol#L42
• Controller.sol#L126
• StakerVault.sol#L259
• BkdLocker.sol#L310
• FeeBurner.sol#L56
• KeeperGauge.sol#L157
• VestedEscrow.sol#L94
• InflationManager.sol#L98
• InflationManager.sol#L112-L121
• InflationManager.sol#L173
• InflationManager.sol#L198
• InflationManager.sol#L267
• InflationManager.sol#L291
• InflationManager.sol#L366
• InflationManager.sol#L390
• InflationManager.sol#L413
• InflationManager.sol#L454

Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• Preparable.sol#L47
• Preparable.sol#L71
• Preparable.sol#L90
• Preparable.sol#L102
• StakerVault.sol#L79
• StakerVault.sol#L84
• StakerVault.sol#L90
• StakerVault.sol#L101
• AddressProvider.sol#L67
• AddressProvider.sol#L74
• AddressProvider.sol#L299
• LpGauge.sol#L120
• KeeperGauge.sol#L61
• KeeperGauge.sol#L89
• KeeperGauge.sol#L99
• KeeperGauge.sol#L116
• VestedEscrow.sol#L86
• AmmGauge.sol#L53
• AmmGauge.sol#L115
• AmmGauge.sol#L137
• AmmGauge.sol#L151
• AmmGauge.sol#L160
• InflationManager.sol#L62
• InflationManager.sol#L72
• InflationManager.sol#L102
• InflationManager.sol#L124
• InflationManager.sol#L142
• InflationManager.sol#L178
• InflationManager.sol#L202
• InflationManager.sol#L233
• InflationManager.sol#L275
• InflationManager.sol#L300
• InflationManager.sol#L318
• InflationManager.sol#L347
• InflationManager.sol#L371
• InflationManager.sol#L395
• InflationManager.sol#L437
• InflationManager.sol#L488
• InflationManager.sol#L578
• InflationManager.sol#L592
• InflationManager.sol#L606

Improve logic

It's possible to optimize the method getRoleMember of the contract RoleManager checking first if the index is 0.

Before:

if (role == Roles.ADDRESS_PROVIDER && index == 0) {
            return address(addressProvider);
        } else if (role == Roles.POOL_FACTORY && index == 0) {
            return addressProvider.getAddress(AddressProviderKeys._POOL_FACTORY_KEY);
        } else if (role == Roles.CONTROLLER && index == 0) {
            return addressProvider.getAddress(AddressProviderKeys._CONTROLLER_KEY);
        } else if (role == Roles.POOL) {
            return addressProvider.getPoolAtIndex(index);
        } else if (role == Roles.VAULT) {
            return addressProvider.getVaultAtIndex(index);
        }

After:

if (index == 0)
{
    if (role == Roles.ADDRESS_PROVIDER) return address(addressProvider);
    if (role == Roles.POOL_FACTORY) return addressProvider.getAddress(AddressProviderKeys._POOL_FACTORY_KEY);
    if (role == Roles.CONTROLLER) return addressProvider.getAddress(AddressProviderKeys._CONTROLLER_KEY);
}
if (role == Roles.POOL) return addressProvider.getPoolAtIndex(index);
if (role == Roles.VAULT) return addressProvider.getVaultAtIndex(index);

Note that also, this change will increase readability.

Affected source code:

• RoleManager.sol#L97-L107

Inline improve

El metodo uncheckedInc de la libreria UncheckedMath pretenden ahorrar gas, pero suponiendo que el compilador lo optimize al ser mayor a 0.8.2 y se compile como inline, aun así devolverá un valor a la pila que puede ser evitado. Y además es más optimo utiliza ++a en lugar de a+1;

The uncheckedInc method of the UncheckedMath library is intended to save gas, but assuming the compiler optimizes it because it's greater than 0.8.2, it will still return a value on the stack that can be avoided. And it is also more optimal to use ++a instead of a + 1;.

Affected source code:

• UncheckedMath.sol#L4-L23