Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #229

Open
code423n4 opened this issue Jun 2, 2022 · 3 comments
Open

QA Report #229

code423n4 opened this issue Jun 2, 2022 · 3 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

code423n4 commented Jun 2, 2022

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 
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 2, 2022
code423n4 added a commit that referenced this issue Jun 2, 2022
@GalloDaSballo
Copy link
Collaborator

Formatting was abysmal please preview your submission

@GalloDaSballo
Copy link
Collaborator

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
Copy link
Collaborator

2 NC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants