Boot Finance contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

contract Vesting defines unixYear as this: uint256 private unixYear = 52 * 7 * 24 * 60 * 60; This is equivalent to 52 weeks or 364 days but to be precise 1 year is always a bit longer, sometimes 52 weeks 1 day (non-leap year), sometimes 52 weeks, and 2 days (leap year), so you lose around 1.25 days of precision here. Also, similar issues exist when defining time constants in other contracts.

Recommended Mitigation Steps

I have 2 suggestions here:

1. define this year more precisely.
2. use built-in variables to improve the readability. Possible solution: uint256 private unixYear = 365.25 days;

Handle

pauliax

Vulnerability details

Impact

Airdrop distribution declares the supply as this: uint256 private airdrop_supply = 20160000 * 10 ** 18; //Total Allocation for Airdrop

However, if you sum up all the airdropBalances, you get 20159997, so 3 tokens are missing somewhere. While this does not cause any serious issue, it makes the calculations a little bit inaccurate when calculating fractions as user percentages will be smaller than they actually should and some tokens will be left in the contract afterward.

Recommended Mitigation Steps

Consider improving the precision of airdrop_supply or airdropBalances to match between.

Handle

pauliax

Vulnerability details

Impact

There is no reason for the function vest to be 'payable' as it does not handle ether in any way and there is no way to rescue it later in case someone accidentally sends it.

Recommended Mitigation Steps

Remove 'payable' from the vest function.

Handle

pauliax

Vulnerability details

Impact

function _recordBurn should validate that _eth > 0. Now it is possible to spam this function with 0 eth burns and fictitiously increase member statistics. I have previously reported this issue in a Vader's contest. You can read find details here: https://github.com/code-423n4/2021-04-vader-findings/issues/269

Recommended Mitigation Steps

Handle case when _eth = 0 in function _recordBurn.

Handle

pauliax

Vulnerability details

Impact

Contracts use assert() instead of require() in multiple places. Assert is recommended to be used to check for internal errors, or to check invariants. In your case, I think these validations could better use 'require' as they are likely to be triggered: assert(claimable > 0); assert(airdrop[msg.sender].amount - claimable != 0); assert(block.timestamp - startEpochTime <= RATE_TIME); assert(block.timestamp - initTime >= YEAR * 5);

A similar issue was submitted in a previous contest and was assigned a severity of low: https://github.com/code-423n4/2021-06-realitycards-findings/issues/83

Recommended Mitigation Steps

Consider replacing 'assert' with 'require' in the cases mentioned above.

Handle

pauliax

Vulnerability details

Impact

function burnEtherForMember should validate that the address of the member is not empty (0x0) to prevent accidental burns.

When adding an investor distribution (function addInvestor) should validate that the total amount is not above the investors_supply. but then you also need to store the total amount that is already assigned to investors.

function modifyInvestor should validate that _investor != _new, otherwise it will delete the investor unless this is an expected feature.

function claimExact should validate that _value > 0 to prevent useless claims.

Handle

pauliax

Vulnerability details

Impact

You do not need to check that uints are >= 0, as they can never go negative.

require(a >= 0 && a <= SwapUtils.MAX_A, "a not within the limits"); require(a2 >= 0 && a2 <= SwapUtils.MAX_A, "a2 not within the limits"); require( futureA >= 0 && futureA <= MAX_A, "futureA must be >= 0 and <= MAX_A" ); require( futureA2 >= 0 && futureA2 <= MAX_A, "futureA2 must be >= 0 and <= MAX_A" );

Handle

pauliax

Vulnerability details

Impact

function validate is very expensive as it iterates over all the airdropArray. You can add an index parameter that can be calculated off-chain to help skip this semi-useless iteration for every user.

Handle

pauliax

Vulnerability details

Impact

'immutable' greatly reduces gas costs. There are variables that do not change so they can be marked as immutable/constant to greatly improve the gast costs. Examples of such variables are: IERC20 public vestingToken; address public multiSig; uint256 private unixYear = 52 * 7 * 24 * 60 * 60;

uint256 private airdrop_supply = 20160000 * 10 ** 18;

uint256 public initTime;

address deployer; address payable burnAddress;

uint public genesis; uint public daysPerEra; uint public firstPublicEra; uint public totalSupply; uint public initialDayEmission;

address tgeContract;

Handle

pauliax

Vulnerability details

Impact

Here, I see at least 2 possible improvements: require(_pooledTokens.length == 2, "_pooledTokens.length must be 2 in length"); require(decimals.length == 2, "decimals.length must be 2 in length"); require( _pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch" ); for (uint8 i = 0; i < _pooledTokens.length; i++)

First, the require of _pooledTokens.length == decimals.length is not necessary as it was already checked that both of these arrays are of length 2. Second, the loop end index can be hardcoded to 2 as again we already know that from the checks above. It would be nice if you extract this hardcoded value of 2 to a constant in case you decide to change it later.

Handle

pauliax

Vulnerability details

Impact

function claim (and probably claimExact) does not need this check: require(msg.sender != address(0)); It is already checked when validating the user (function validate) and not validated users cannot claim.

Handle

pauliax

Vulnerability details

Impact

Using the unchecked keyword to avoid redundant arithmetic underflow/overflow checks to save gas when an underflow/overflow cannot happen. We can apply the unchecked keyword in the following lines of code since there are require statements before to ensure the arithmetic operations would not cause an integer underflow or overflow:

airdrop[msg.sender].amount -= claimable; // because assert(airdrop[msg.sender].amount - claimable != 0); uint256 claimable_not_yet_vested = claimable - claimable_to_send;

require(airdrop[msg.sender].amount >= claimable); require(_value <= claimable); airdrop[msg.sender].amount -= _value;

mainToken.approve(address(vestLock), 2**256-1);

Carefully review all the operations and consider using the 'unchecked' keyword where it is safe to save gas.

Handle

pauliax

Vulnerability details

Impact

functions available_supply() and _available_supply() are identical, the former even has a misleading comment that it is an internal function but it is actually private. Nevertheless, these functions are duplicates and this makes it harder to maintain in case you want to change the calculation logic later so you should consider re-using them.

Handle

pauliax

Vulnerability details

Impact

No need to set revocable to false as it is a default value that will be set anyway: if(_isRevocable == 0){ benRevocable[_beneficiary] = [false,false]; } else if(_isRevocable == 1){ benRevocable[_beneficiary] = [true,false]; }

Proposed improvement: if (_isRevocable == 1) { benRevocable[_beneficiary] = [true, false]; }

Handle

pauliax

Vulnerability details

Impact

contracts AirdropDistribution, InvestorDistribution, PublicSale, and Vesting are using SafeMath library even though they declare a pragma of version >0.8 which brings safe math by default. Using SafeMath operations is redundant in such a case.

Handle

pauliax

Vulnerability details

Impact

There are places where the same storage variable is accessed multiple times in the same function. It would be more gas efficient to cache these variables and re-use them where necessary. E.g. function modifyInvestor accesses investors[_investor] 6 times. function _claimableAmount reads from timelocks[_addr] several times during the iterations but because this function does not modify the timelocks[_addr], it can cache this struct before executing the loop and only read cached values inside. Or here it reads the same value from storage 2 times: if (airdrop[msg.sender].claimed != 0) { claimable -= airdrop[msg.sender].claimed; } This is a waste of precious gas.

Handle

pauliax

Vulnerability details

Impact

This check in function modifyInvestor is not neccessary: require(_investor != address(0), "Invalid old address");

as empty address cannot be added in function addInvestor and later this check will fail: require(investors[_investor].amount != 0);

Handle

pauliax

Vulnerability details

Impact

function claim can save gas and eliminate duplicate storage access and math operations by caching claimableAmount and re-using it later when setting the benClaimed.

before: uint256 amount = _claimableAmount(msg.sender).sub(benClaimed[msg.sender]); ... benClaimed[msg.sender] = benClaimed[msg.sender].add(amount);

after: uint256 claimableAmount = _claimableAmount(msg.sender); uint256 amount = claimableAmount.sub(benClaimed[msg.sender]); ... benClaimed[msg.sender] = claimableAmount;

Also, it looks strange that in function revoke the amount is checked with 'assert': assert(amount <= benTotal[_addr]); but in function claim 'require' is used: require(amount <= benTotal[msg.sender], "Cannot withdraw more than total vested amount");

In both places probably 'assert' should be used as it is checking a scenario that should never happen under normal circumstances.

Handle

pauliax

Vulnerability details

Impact

When revoking the user, there is no need to iterrate over all his timelocks again and calculate the total amount as it should already be stored in a benTotal[_addr] mapping: uint256 locked = 0; for (uint256 i = 0; i < timelocks[_addr].length; i++) { locked = locked.add(timelocks[_addr][i].amount); }

Handle

pauliax

Vulnerability details

Impact

Member total_tokens in both structs Airdrop and Investors is practically not used and is a duplicate of the amount so you can remove it to save some storage. Also, gas efficiency can be improved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage, e.g. airdropBalances which are later translated to the amount in Airdrop struct (10**18) can be stored in a smaller version of uint as we know all the exact values at compile time.

Handle

pauliax

Vulnerability details

Impact

functions validate and modifyInvestor do not need nonReentrant modifier as they do not execute any external calls where you can hook up to re-enter.

Handle

pauliax

Vulnerability details

Impact

function _recordBurn does not really need this parameter of address _payer as it is always equal to msg.sender.

Consider replacing: function _recordBurn(address _payer, ... emit Burn(_payer, ...

with: function _recordBurn(... emit Burn(msg.sender, ...

Handle

pauliax

Vulnerability details

Impact

There are some variables that are never used. Remove them to save some gas or use where they were intended. Examples are: mapping(address => uint) private _balances; mapping(address => mapping(address => uint)) private _allowances;

uint public constant firstEra = 1;

uint256 constant HOUR = 3600; uint256 constant DAY = 86400;

Handle

pauliax

Vulnerability details

Impact

Normally there's no benefit to using these sub-types because Solidity reserves 256 bits of storage regardless of the uint size. E.g. using uint8 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs. There are many places where you use e.g. uint8, a few concrete examples: mapping(address => uint8) private tokenIndexes; for (uint8 i = 0; i < _pooledTokens.length; i++) function getTokenBalance(uint8 index) uint8 public constant POOL_PRECISION_DECIMALS = 18;