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

0 stars 0 forks source link

QA Report #208

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

looping though storage not good idea make into memory or calldata and can even make into a dos. https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L1226 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L1259 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L1285 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L1332 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L1359 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Router.sol#L316 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/RewardsDistributor.sol#L301 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Gauge.sol#L353

1.dont use block.number 2.dont just use iscontract function because you can have a contract in the constrocutr and its no code size yet so use is contract and tx.orgain because tx.origan makes sure its eoa account. 2.make sure the converting uint to int that there can no chance of overflow and get a lower number you can have calculation go wrong Like converting uint8 to int8 if uint is 129 and change to int its -2 because int8 goes up to 127 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/VotingEscrow.sol#L932 3.make all the require revert and for users make alot of your require or revert put in a error code or error string 4.already have safe functions (approve,transferfrom)in other contract either make a base contract to derive from or make into one contract and import that also gas to https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Gauge.sol#L663-L684 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Bribe.sol#L92-L105 Duplicated code above and lock of reentrancy lock in multiple contracts take it out and derive from all. https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Voter.sol#L66-L72

5.the initializer function can be frontrun and you can effect the logic of contract and may have to deploy again https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/Voter.sol#L74 https://github.com/code-423n4/2022-05-velodrome/blob/731a7c438c4f93efc8310586d217006930be63fd/contracts/contracts/redeem/RedemptionReceiver.sol#L37

GalloDaSballo commented 2 years ago

looping though storage not good idea make into memory or calldata and can even make into a dos.

-> Some of the entries are Memory, finding is invalid in lack of POC and details

1.dont use block.number

Why?

2.dont just use iscontract function because you can have a contract in the constrocutr and its no code size yet so use is contract and tx.orgain because tx.origan makes sure its eoa account.

Not valid in lack of POC

Like converting uint8 to int8 if uint is 129 and change to int its -2 because int8 goes up to 127

Link has no conversions

3.make all the require revert and for users make alot of your require or revert put in a error code or error string

Valid NC

4.already have safe functions (approve,transferfrom)in other contract either make a base contract to derive from or make into one contract and import that also gas to

Because we know the implementation of the tokens the finding is not valid

Duplicated code above and lock of reentrancy lock in multiple contracts take it out and derive from all.

Valid NC

5.the initializer function can be frontrun and you can effect the logic of contract and may have to deploy again

You cannot frontrun either of these.

This report is:

I'm not going to mark as invalid, but please take notes and do better next time