Backd Tokenomics contest - oyc_109's results

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmGauge.sol#L130-L135

Vulnerability details

description

The unstakeFor function of the AmmGauge contract is updating the balances of msg.sender after a token transfer.

Solidity recommends the usage of the Check-Effects-Interaction Pattern to avoid potential security issues, such as reentrancy.

The unstakeFor function can be used to conduct a reentrancy attack, if the ammToken is token with a hook on receive, such as ERC777

/2022-05-backd/protocol/contracts/tokenomics/AmmGauge.sol
130:         uint256 oldBal = IERC20(ammToken).balanceOf(address(this));
131:         IERC20(ammToken).safeTransfer(dst, amount);
132:         uint256 newBal = IERC20(ammToken).balanceOf(address(this));
133:         uint256 unstaked = oldBal - newBal;
134:         balances[msg.sender] -= unstaked;
135:         totalStaked -= unstaked;

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

array lengths should be cached

/2022-05-backd/protocol/contracts/access/RoleManager.sol
82: for (uint256 i; i < roles.length; i = i.uncheckedInc()) {

/2022-05-backd/protocol/contracts/RewardHandler.sol
42: for (uint256 i; i < pools.length; i = i.uncheckedInc()) {
259: for (uint256 i; i < actions.length; i = i.uncheckedInc()) {

/2022-05-backd/protocol/contracts/tokenomics/FeeBurner.sol
56: for (uint256 i; i < tokens_.length; i = i.uncheckedInc()) {

/2022-05-backd/protocol/contracts/tokenomics/InflationManager.sol
116: for (uint256 i; i < stakerVaults.length; i = i.uncheckedInc()) {

/2022-05-backd/protocol/contracts/tokenomics/VestedEscrow.sol
94: for (uint256 i; i < amounts.length; i = i.uncheckedInc()) {

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

findings

/2022-05-backd/protocol/contracts/tokenomics/FeeBurner.sol
43: function burnToTarget(address[] memory tokens_, address targetLpToken_)

named returns and a return statement isn’t necessary

description

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

findings

/2022-05-backd/protocol/contracts/tokenomics/FeeBurner.sol
47: returns (uint256 received)

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

/2022-05-backd/protocol/contracts/tokenomics/KeeperGauge.sol
98: epoch++;

use custom errors

description

use custom errors instead of revert strings

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

eg

/2022-05-backd/protocol/contracts/tokenomics/VestedEscrow.sol
82: require(!initializedSupply, "Supply already initialized once");