Platform: Code4rena
Start Date: 04/11/2021
Pot Size: $50,000 USDC
Total HM: 20
Participants: 28
Period: 7 days
Judge: 0xean
Total Solo HM: 11
Id: 51
League: ETH
Rank: 10/28
Findings: 4
Award: $1,086.67
🌟 Selected for report: 2
🚀 Solo Findings: 0
leastwood
The Vesting.vest() function is called by airdrop/investor distributions to lock 70% of their token allocations for a period of one year. Vestings are defined on a linear schedule and can be claimed as often as the user likes. However, the _claimableAmount() function iterates over the list of vestings for a particular user to calculate claimable tokens. As a result, a malicious user could call vest() multiple times with an arbitrarily small amount and effectively deny airdrop/investor distributions. If the number of tokens to be distributed is significant, it may prove useful for other recipients to deny these users from receiving their tokens in order to increase the purchasing power of their own holdings. I categorise this issue as high risk due to potential loss of investor/airdrop holdings.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L73-L98 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L162-L188
Manual code review. Discussions with dev.
Ideally avoid iterating over a list of vestings or alternatively force the user to define a list of indexes that point to which vestings they would like to claim. Additionally, it may prove useful to also prevent the vest() function from being called by any contract besides AirdropDistribution.sol and InvestorDistribution.sol.
#0 - chickenpie347
2021-11-16T14:21:28Z
Addressed in #120
leastwood
The onlyOwner role typically calls revoke() if a member leaves the BootFinance team, resulting in vested tokens being transferred to the multisig account. Each vesting account has a revocable state variable that is set to either true or false. As any user can arbitrarily call vest() it may prove useful for team members to protect their vestings by calling vest() where _isRevocable is set to 0, thereby preventing the onlyOwner role from calling revoke(). This is unintended as it limits the functionality of revoke(), hence I would make this issue medium risk.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L73-L98 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L104-L137
Manual code review.
Consider preventing benRevocable from being changed if a user calls the vest() function again. Ideally, if the vest() function is limited to the airdrop/investor distribution contracts and a multisig account, it should prevent any malicious behaviour from users attempting to game the Vesting.sol contract.
#0 - chickenpie347
2021-11-16T13:57:19Z
Addressed in #132.
🌟 Selected for report: leastwood
290.7617 USDC - $290.76
leastwood
The claim() function asserts that the claimable amount is strictly less than benTotal for a given user. However, this does not take into account previously claimed tokens, hence the require does not accurately depict its intended behaviour.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L197
Manual code review.
Consider updating this require statement to account for already claimed tokens. This could look like the following:
require(amount.add(benClaimed[msg.sender]) <= benTotal[msg.sender], "Cannot withdraw more than total vested amount");
leastwood
The assert keyword is used in several instances within the BootFinance smart contract suite. This keyword will revert a transaction in the event the condition is not satisfied, however, no gas is refunded to the user. As a result, this may lead to poor user experience and other issues if the supplied gas/gas price is considerably high.
https://github.com/code-423n4/2021-11-bootfinance
Manual code review. Best solidity practises.
Consider replacing all instances of the assert keyword with require.
#0 - chickenpie347
2022-01-04T03:05:48Z
Duplicate of #279
🌟 Selected for report: leastwood
44.8876 USDC - $44.89
leastwood
The Timelock struct is used to reference the releaseTimestamp and vested amount for each vesting. These values can likely be safely stored as uint64 and uint192 values respectively, enabling the struct to be stored within a single slot instead of two slots.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L32-L35
Manual code review.
Consider updating releaseTimestamp to uint64 and amount to uint192 within the Timelock struct. It might be worthwhile performing sanity checks when storing these values by using OpenZeppelin's safe math and safe cast libraries.
leastwood
The following functions are declared using the public keyword, exposing the function internally when there are no internal calls made.
https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L104 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/Vesting.sol#L142 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/AirdropDistribution.sol#L595 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/AirdropDistribution.sol#L607 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/InvestorDistribution.sol#L185 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/InvestorDistribution.sol#L197 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/InvestorDistribution.sol#L203 https://github.com/code-423n4/2021-11-bootfinance/blob/main/vesting/contracts/InvestorDistribution.sol#L212
Manual code review.
Updating these functions to use the external keyword can generate considerable gas savings upon deployment and on each function call.
#0 - chickenpie347
2022-01-04T03:06:21Z
Duplicate of #121