review comments

Author: mskhno

src/Governor.sol

@@ -229,6 +229,8 @@ contract Governor is EIP712 {
      * @dev Values sent to the contracts are sent by the Timelock contract
      */
     //@audit vlad. почему это payable?
+    // @comment maks. если в пропоузал есть транзакция с переводом эфира. честно не тестил, интуитивно так подумал
+    // @comment сейчас подумал, что даже если так, то высылать эфир будет Timelock, а не Governor
     function execute(uint256 proposalId) external payable {
         if (state(proposalId) != ProposalState.Queued) {
             revert Governor__ProposalIsNotQueued();
@@ -307,13 +309,17 @@ contract Governor is EIP712 {
      * @dev Maybe add address voter to Ballot struct and check if signer == voter
      */
     function castVoteBySig(uint256 proposalId, bool support, uint8 v, bytes32 r, bytes32 s) external {
-//@audit vlad. Не, шанс коллизии слишком маленький. Из-за парадокса дней рождений шанс 1 коллизии 50% за 2^80 попыток
-// По фану можешь посмотреть с какой скоростью твой ноут печатает числа в цикле
-// При этом коллизия любых адресов, а делегаты 1 токена это супер мало адресов. Потом скину дискуссию по этому поводу (сейчас инета нету)
+        //@audit vlad. Не, шанс коллизии слишком маленький. Из-за парадокса дней рождений шанс 1 коллизии 50% за 2^80 попыток
+        // По фану можешь посмотреть с какой скоростью твой ноут печатает числа в цикле
+        // При этом коллизия любых адресов, а делегаты 1 токена это супер мало адресов. Потом скину дискуссию по этому поводу (сейчас инета нету)
+
+        // @comment maks. когда пишу код с подписями, моё натуральное желание это впихнуть в подпись как можно больше вещей.
+        // тут я подумал об этом, но не смог сузить вот так границу как ты.
+
         // any checks at all? can it cast a random persons vote in case of random signature spam? sounds like i am not really understading something, missing out
-        // возможно ли в теории наспамить в эту функицю кучу подписей, с идеей что хоть одна попадется, 
-    // в который я угадаю параметры proposalId и support и при этом signer это участник DAO и он делегировал токены,
-    //чтобы его голос засчитался? трудно наверно но мозг мой вот так подумал
+        // возможно ли в теории наспамить в эту функицю кучу подписей, с идеей что хоть одна попадется,
+        // в который я угадаю параметры proposalId и support и при этом signer это участник DAO и он делегировал токены,
+        // чтобы его голос засчитался? трудно наверно но мозг мой вот так подумал
         // add address voter to the signature and the check signer == voter?
         bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
         bytes32 digest = _hashTypedDataV4(structHash);
@@ -386,9 +392,18 @@ contract Governor is EIP712 {
     ) internal {
         bytes32 txHash = keccak256(abi.encode(proposalId, target, value, signature, data, eta));
 
-//@audit vlad. С комментарием согласен
-// Также хочу заметить, что в нынешнем дизайне proposal не сможет содержать 2 одинаковых действия
-// То есть у них будут одинаковые параметры и одинаковый хэш. Вроде бы у Compaund это свойство есть тоже
+        //@audit vlad. С комментарием согласен
+        // Также хочу заметить, что в нынешнем дизайне proposal не сможет содержать 2 одинаковых действия
+        // То есть у них будут одинаковые параметры и одинаковый хэш. Вроде бы у Compaund это свойство есть тоже
+
+        // @comment maks. то, что нельзя вручную в очередь поставить транзакцию это да, поэтому тут чек отлетает
+        // "что в нынешнем дизайне proposal не сможет содержать 2 одинаковых действия" - не думал так об этом.
+        // по сути строка 393 одинаковый бы txHash посчитала, но тогда как раз нужен этот чек?
+        // вижу тут это так - ситуация если этот чек убрать. queue() в своем for цикле поставит в очередь 2 транзакции,
+        // но по итогу в mappinge queuedTransactions будет только одна. при execution пропоузала execute() вызовет executeTransaction() на эти две одинаковые,
+        // и по идее Timelock ревертнет на линии 103, так как транзакция уже не в очереди, а txHash у второй такой транзакции такой же как у первой
+        // по сути наебнется весь пропоузал?
+
         // may be unnecessary check, because there is no way to manually queue a transaction in the timelock contract
         // as a check for collision, it's really unlikely since txHash includes proposalId and proposal.eta
         if (i_timelock.queuedTransactions(txHash)) {
@@ -444,6 +459,7 @@ contract Governor is EIP712 {
     function _checkProposalMeetsQuorum(uint256 proposalId) internal view returns (bool) {
         Proposal storage proposal = proposals[proposalId];
         //@audit vlad. Precision loss не будет, ну или он слишком малый
+        // @comment maks. мало у меня уверенности в solidity math
         uint256 quorumAmount = (i_token.getPastTotalSupply(proposal.startBlock - 1)) * quorumVotes() / 100; // precision loss?
 
         if (proposal.forVotes + proposal.againstVotes >= quorumAmount) {
@@ -495,6 +511,7 @@ contract Governor is EIP712 {
      * @return uint256 The threshold of votes needed to propose
      */
     //@audit vlad. Не учитывает что токен имеет 18 decimals, считай proposalThreshold и нету
+    // @comment maks. так же не уверен в использовании decimals
     function proposalThreshold() public pure returns (uint256) {
         return 1000;
     }

src/Timelock.sol

@@ -105,6 +105,7 @@ contract Timelock is ITimelock, Ownable {
         }
 
         //@note можно запустить только в период [eta; GRACE_PERIOD]
+        // @comment maks. имеешь в виду [eta; eta + GRACE_PERIOD], и разве не ) в конце?
         if (block.timestamp < eta) {
             revert Timelock__DelayHasNotPassed();
         } else if (block.timestamp >= eta + GRACE_PERIOD) {
@@ -124,6 +125,10 @@ contract Timelock is ITimelock, Ownable {
         // Сейчас оффлайн, поэтому не приведу примеры issues из репортов
         // Это было в Moonwell на C4, JOJO на Sherlock и ещё пару раз
         // Но это распространённая ошибка, забывают `payable receive()` добавить
+
+        // @comment maks. тру. так же не уверен с этими receive() и fallback() функции
+        // технически, сейчас Timelock не может иметь эфир, потому что его сюда нельзя прислать?
+        // это можно было бы заэксплоитить?
         (bool success,) = target.call{value: value}(callData);
         if (!success) {
             revert Timelock__TransactionExecutionReverted();