code-423n4 / 2022-05-vetoken-findings

1 stars 1 forks source link

QA Report #229

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Headline

Not having emits on critical functions https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L90

emit LockUpdated(veAssetBalanceStaker, unlockTime mayabe emit also the unlockat so you can query the data can be helpful

if this doesn't fail its dead code because veaset staker can only be zero either on deployment or on first call to this function uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker); if (veAssetBalanceStaker == 0) { return; } This code is very rare to happen because to get into the system balance of staker is more then zero

Precision error multiplication first because you can have presion errors uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;

if (veVeAsset == 0) { uint256 unlockAt = block.timestamp + maxTime; uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;

       //release old lock if exists
       IStaker(staker).release();
       //create new lock
       uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker);
       IStaker(staker).createLock(veAssetBalanceStaker, unlockAt);
       unlockTime = unlockInWeeks;
       emit InitialLockCreated(veAssetBalanceStaker, unlockInWeeks);
   }

You can easily skip this logic because it only executes veveAsset ==0 is very rare. just like the case above ^

comment on how this function works with comments, there are not comments or natspec how this function works Lockasset function

IStaker(staker).increaseTime(unlockAt); add to make sure that if succeeds and the function that is not updating anything in this function make it in require statement

unlocktime = unlock weeks have a require statement here and make sure it happens here you are doing unnecessary state variable make into memory variable issue make this more readable because user might not know what is going on you're not saying why it's reverting require(_amount > 0, "!>0");

no check for address zero https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L130

and no return, not good for other contracts implementing this function https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L130

if (_lock) { https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L134

         dont give user able to lock there should be onlylock  modifer so you dont have a choice on what ot do or you can make  2 functions one for locking and the other 
GalloDaSballo commented 2 years ago

Formatting was abysmal please preview your submission

GalloDaSballo commented 2 years ago

Not having emits on critical functions

NC

if this doesn't fail its dead code because veaset staker can only be zero either on deployment or on first call to this function

I don't understand this, please try to write in more detail next time

 Precision error multiplication first because you can have presion errors

Invalid, this is not a mistake, they are using modulo math

comment on how this function works with comments, there are not comments or natspec how this function works

NC

I genuinely cannot understand the rest, please try to write simpler sentences

GalloDaSballo commented 2 years ago

2 NC