Boot Finance contest - pants's results

Handle

pants

Vulnerability details

An admin can (by mistake maybe) addInvestor with address that already exists. This way its funds are locked in the system and cannot be withdrawn, even by the admin.

Handle

pants

Vulnerability details

The functions ETHPoolDelegator.balances() and ETHPoolDelegator.coins() accept an argument called i and use it as an index to determine which element in the _balances / _coins array should be loaded and returned. However, these functions don't check that the index they receive as an argument actually fits the bounds of the array.

Impact

If the index exceeds the array length, there will be a revert with no informative error message. The user wouldn't know what caused the revert.

Tool Used

Manual code review.

Recommended Mitigation Steps

Add an appropriate require statement to each of these functions to validate that the given argument fits the _balances / _coins array bounds.

Handle

pants

Vulnerability details

The functions USDPoolDelegator.balances(), USDPoolDelegator.coins() and USDPoolDelegator.underlying_coins() accept an argument called i and use it as an index to determine which element in the _balances / _coins / _underlying_coins array should be loaded and returned. However, these functions don't check that the index they receive as an argument actually fits the bounds of the array.

Impact

If the index exceeds the array length, there will be a revert with no informative error message. The user wouldn't know what caused the revert.

Tool Used

Manual code review.

Recommended Mitigation Steps

Add an appropriate require statement to each of these functions to validate that the given argument fits the _balances / _coins / _underlying_coins array bounds.

Handle

pants

Vulnerability details

At LPToken.sol the state variable swap could be set immutable. swap is defined in line 20.

Handle

pants

Vulnerability details

optimizing for loops by using unchecked{++i} over i++

For example in line 535 of SwapUtils.sol you have

    for (uint256 i = 0; i < numTokens; i++) {

While a more effiient looping would be:

    for (uint256 i = 0; i < numTokens; unchecked{++i}) {

One cannot reach uint256 maximal value so it's overflow safe.

Handle

pants

Vulnerability details

Avoiding initialization of loop index to save gas since it's already initialized to 0. For example in line 116 of vesting.sol.

This is also happening in many other places (>10) of the code.

Handle

pants

Vulnerability details

The functions BTCPoolDelegator.balances() and BTCPoolDelegator.coins() accept an argument called i and use it as an index to determine which element in the _balances / _coins array should be loaded and returned. However, these functions don't check that the index they receive as an argument actually fits the bounds of the array.

Impact

If the index exceed the array length, there will be a revert with no informative error message. The user wouldn't know what caused the revert.

Tool Used

Manual code review.

Recommended Mitigation Steps

Add an appropriate require statement to each of these functions to validate that the given argument fits the _balances / _coins array bounds.

Handle

pants

Vulnerability details

Some function of USDPoolDelegator that declared public could be set external since there are no in-contract calls to them. For example the 'balances' function.

Handle

pants

Vulnerability details

Impact

In BTCPoolDelegator contract, address state variables future_owner in lines 51 and bool state variable is_killed in line 55 should be placed one after another. solidity keep storage in 32 bytes slot and can optimize multiple variables that are less than 32 bytes. address is 20 bytes and bool is 1 byte, so it can be placed in one storage slot instead of two.

Proof of Concept

Tested it on Remix, saves 50 gas per transaction

Recommended Mitigation Steps

change

50    uint256 public future_admin_fee;
51    address public future_owner;
52
53    uint256 kill_deadline;
54    uint256 constant kill_deadline_dt = 2 * 30 * 86400;
54    bool is_killed;

to

50    uint256 public future_admin_fee;
51    uint256 constant kill_deadline_dt = 2 * 30 * 86400;
52
53    uint256 kill_deadline;
54    address public future_owner;
54    bool is_killed;

Handle

pants

Vulnerability details

When double reading a variable from memory the gas efficient way is to cache it and use the cached value.

For example in line 537, 538 of SwapUtils.sol you have two accesses to xp[i]. You can save xp[i] as local variable and then use it instead at the second time.

Handle

pants

Vulnerability details

The function PoolGauge.withdraw() doesn't have to execute these lines of code when _value equals zero:

_balance: uint256 = self.balanceOf[msg.sender] - _value
_supply: uint256 = self.totalSupply - _value
self.balanceOf[msg.sender] = _balance
self.totalSupply = _supply

self._update_liquidity_limit(msg.sender, _balance, _supply)

assert ERC20(self.lp_token).transfer(msg.sender, _value)

Impact

There is no reason to execute these lines of code if _value equals zero because they won't affect the system. An identical optimization is already implemented in PoolGauge.deposit().

Tool Used

Manual code review.

Recommended Mitigation Steps

Execute these lines of code only if _value doesn't equal zero.

Handle

pants

Vulnerability details

In InvestorDistribution at line 19 you have

using SafeMath for uint256;

although you use solidity >0.8.0 therefore you don't need to use safeMath for uint256

Handle

pants

Vulnerability details

In InvestorDistribution you generally use uint256 for every quantity, although 256 bits are much more than necessary. For example for Investor structure you can change all to uint128, and for every normal use it will not affect and save gas.

Handle

pants

Vulnerability details

Consider the following require statement:

// condition is boolean // str is a string require(condition, str) The string str is split into 32-byte sized chunks and then stored in memory using mstore, then the memory offsets are provided to revert(offset, length). For chunks shorter than 32 bytes, and for low --optimize-runs value (usually even the default value of 200), instead of push32 val, where val is the 32 byte hexadecimal representation of the string with 0 padding on the least significant bits, the solidity compiler replaces it by shl(value, short-value)). Where short-value does not have any 0 padding. This saves the total bytes in the deploy code and therefore saves deploy time cost, at the expense of extra 6 gas during runtime. This means that shorter revert strings saves deploy time costs of the contract. Note that this kind of saving is not relevant for high values of --optimize-runs as push32 value will not be replaced by a shl(..., ...) equivalent by the Solidity compiler.

Going back, each 32 byte chunk of the string requires an extra mstore. That is, additional cost for mstore, memory expansion costs, as well as stack operations. Note that, this runtime cost is only relevant when the revert condition is met.

Overall, shorter revert strings can save deploy time as well as runtime costs.

Note that if your contracts already allow using at least Solidity 0.8.4, then consider using Custom errors. This is more gas efficient, while allowing the developer to describe the errors in detail using NatSpec. A disadvantage to this approach is that, some tooling may not have proper support for this, as well as block explorers like Etherscan.

Examples Here is a non-exhaustive list of lengthy revert strings that could be shortened:

Vesting.sol line 197 Swap.sol line 149

Handle

pants

Vulnerability details

Removing unused named return variables can reduce gas usage and improve code clarity.

For example, FairSideDAO.sol line 275 You both use named return and actual return statement. We recommend to use only one of them as explained.

Look at this issue: https://github.com/code-423n4/2021-10-tempus-findings/issues/4

Handle

pants

Vulnerability details

You import "import "./hardhat/console.sol";" and all uses are commented. You should also comment the import. SwapUtils line 9

Handle

pants

Vulnerability details

In InvestorDistribution line 103 you read investors[_investor] multiple times (only at this function investors[_investor] is read 6 times). You could cache the value and call the cached value instead. Investors x = investors[_investor]; And then use x.amount, x.claimed, etc.

Handle

pants

Vulnerability details

On L158 of swap.sol, you use a uint8 as the for loop variable:

Due to how the EVM natively works on 256 numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage

Handle

pants

Vulnerability details

Since no contract inherent from SwapUtils all internal functions could be set private. For example getD could be set private to save gas.

Handle

pants

Vulnerability details

optimizing for loops by caching memory array length, instead of calling it every time.

For example at Swap.sol at time 158 you should have

uint8 len = _pooledTokens.length and in the next line define the forloop with stop condition of i<len

This appears in many places in the code. At some places you did cached the array length correctly.

Handle

pants

Vulnerability details

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is much cheaper in solidity. Basically, Any fixed size variable in solidity is cheaper than variable size. On the MarketPlace.sol contract, string memory variable can be replaced with bytes32 array. That will save gas on the contract.

An example is revert messages. For example look at line 32 of PublicSale.sol.