Sp Eth Vesting 5 (test audit)

# Sp Eth Vesting 5 (test audit)  ###### tags: `Audit` ## Project Info * repo - `-` * commit - `944d09fbd77149747739fc99f05397803aa7a0ee` * scope - `InsidersVesting.sol` * SLOC-P - 170 * documentation - `-` * link - `-` ## Tools :::warning Отмечаем запущенные тулы. Если тул не запустился/..., указываем краткий коммент ::: * [x] Slither * [ ] SmartCheck ## Project Issues :::warning Проверяем по [полному чеклисту](https://docs.google.com/document/d/1HVqBsK--HhMjD7BF9Al-6IFFMLbbx4ui49L8CQYvftc/edit#), при речеке смотрим изменения. ::: * documentation (med) - no documentation. No NatSpec comments. * compilation - OK * tests - 38/38 OK, coverage 100% * dependency management - OK ## Code Issues * `InsidersVesting.initialize` L95 (medium, code) - Incorrect strict equality. If anyone is able to transfer tokens to the contract and not become a beneficiary, `initialize` function will revert and the initialization will be impossible. * `InsidersVesting.claim` L113 (low, erc20, erc20return) - ERC20 `transfer` return value is not checked. * `InsidersVesting.initialize` (low, events) - Events maths. There is no event emitted on initializing the contract. The best practices support emitting events on major actions. * `InsidersVesting.initialize` L91 (low, gas) - Unnesessary `require` statement. If `tokensLimitRemaining` was less than `b.tokenAmount`, the call would revert on line 92. * `InsidersVesting.transfer` (low, code) - No zero value checks. The function allows beneficiaries to set both transfer values to zero and create an "empty" beneficiary with no vesting tokens. Consider following the best practices and checking that at least one of the values is non-zero unless you have a reason not to. ### Properties to check :::warning Записываем все вопросы, возникающие по ходу аудита. Потом дописываем `// комментарий с объяснением` и отмечаем чекбокс. ::: * [x] При initialize число токенов на контракте должно быть равно сумме токенов, которые лочат бенефициары. Может ли это быть проблемой? // да. Это ишью * [x] `lastVestingUpdate == 0`. Что именно значит эта проверка? // она означает, что нет бенефициара с таким адресом (структура пустая), т.к. нигде это поле не может стать нулем. * [x] `BeneficiaryInfo.startTime` меняется после трансфера. Почему? // startTime обозначает, когда был добавлен бенефициар. Он может быть создан после трансфера на его адрес (обновление "баланса") * [x] Проверить все сравнения `block.timestamp` с отметками времени * [x] Проверить, что после окончания вестинга число токенов, которые можно вывести, не превзойдет начальное число токенов. * [x] Проверка, что `to` и `from` при трансфере это бенефициары. // `to` это не всегда бенефициар. Если нет, то бенефициар создается для этого адреса. `from` - всегда. ## High level observations :::warning Мы хотим давать общую оценку проекта и выписывать все наши другие замечания ::: * code quality - // poor/mediocre/good/great/etc. * concerns - // ограниченный скоуп, сложная логика или структура кода, дурацкая архитектура, все что угодно * recommendation - // документация, натспеки, тесты, поменять команду разработки ## Appendix ### Project description :::warning Заполняем в начале аудита, при необходимости устраиваем созвон. Можно на русском языке =) Синие блоки служат примером, их надо удалять, чтобы не раздувать мд. ::: #### High level description Business description of the project, one-two sentences. * `InsidersVesting` - Контракт, позволяющий вестить токены * Токен выбирается при инициализации владельцем контракта. Тогда же выбираются бенефициары вестинга * Весь период вестинга делится на два промежутка: lockup период и период начисления. * В lockup период не открываются токены, которые можно вывести. В этот период также нельзя вывести токены, но можно бенефициары могут трансферить токены между собой. Период длится 90 дней * В период начисления будут постепенно открываться токены. В этот период можно выводить токены, которые открылись. Период длится 33 месяца * Период начала вестинга определяется при инициализации * При трансферах внутри вестинга не происходит трансфера токенов - просто меняются состояния бенефициаров (tokensUnlocked, tokensLocked) * Перед инициализацией контракт уже должен содержать все токены пользователей, которые будут веститься. * При трансфере `InsidersVesting.transfer` получатель становится бенефициаром, если он им не является. Т.е. бенефициар всегда имеет возможность добавить столько бенефициаров, сколько позволяет его баланс в контракте. #### Main actors, actions, flows List of roles and their main actions. * Owner - initializes a start of vesting * Beneficiary - участник вестинга. Его токены лочатся и открываются в течение периода начисления. Может выводить токены. #### Technical details Design and architecture choices, clever tricks and hacks, complex logic, etc. * tokensPerSecond меняется при трансфере. * `lastVestingUpdate` обозначает, когда последний раз актуализировались `tokensUnlocked`. Это происходит при `_calculateClaimAndStage` и при трансфере. * `BeneficiaryInfo.startTime` показывает, когда бенефициар был добавлен. Нужно для фронтенда, судя про всему - в контракте не используется. * All balances are scaled to token's decimals. They are uint96, which should be enough: 2^96/10^18 = 7,9*10^10