GalloDaSballo
commented
2 years ago Approve race condition
The more I see this finding, the more I think it's invalid.
The finding is saying that we should set allowance to 0, however setting it to 0 mid-transaction would have absolutely no impact.
Forcing allowance to be set to 0 is something that the end user should do to avoid the race condition, however that is not something that the smart contract can avoid, predict or change.
For those reasons I'm not going to count the finding
Centralized risks
Would disagree with this being a risk and more a property of the system, worth sharing with end users though
Unsecure _safe[ERC20call] implementation
Disagree with he finding, if anything this finding is solving for both "true" as return as well as no return value, which more typically is flagged in C4 contests
Packages with vulnerabilities
Unique finding, worth sharing with the sponsor as supply chain attacks can be executed via old dependencies
Inconsistent solidity pragmas
Finding seems to be valid given there's no locked pragma in foundry settings
Lack of input checks
Valid per industry standards
Unsafe ERC20 calls
Because all 3 tokens resolve to _ve which is going to be Velo, of which the implementation is known and under scope, I believe the finding to have no severity.
Unreal comment
Finding is valid, the comment is not corresponding to what the code does, the code is more open-ended than what the comments leads to believe
Lack of ACK during owner change
Finding is non-critical
Possible unsync Gauge
Finding is not valid as notifyRewardAmount does exactly that
Reentrancy pattern
The sponsor used lock to avoid reEntrancy
Coding style
No strong opinion on this one, if anything I'm not sure why you'd use this pattern
Open TODO
Comment is valid
All in all the warden has found some unique findings, some others feel like copy pasted
4 Low 2 NC
Low
Approve race condition
VeloandPaircontracts does not have any protection against the well-known “Multiple Withdrawal Attack” attack on the Approve/TransferFrom methods of the ERC20 standard.Although this attack poses a limited risk in specific situations, it is worth mentioning to consider it for possible future operations.
There are solutions to mitigate this front running such as, to first reduce the spender's allowance to 0 and set the desired value afterwards; another solution could the one that Open Zeppelin offers, where the non-standard decreaseAllowance and increaseAllowance functions have been added to mitigate the well-known issues involving setting allowances.
Recommended:
Affected source code:
Centralized risks
Until the admin sets the gauge, the
Bribe.notifyRewardAmountis denied, it cannot be a reward token, and theIGauge(gauge).addBribeRewardToken(token);call will fail.Affected source code:
Unsecure _safe[ERC20call] implementation
The methods
safeTransferandsafeTransferFromand_safeApprovehas the following condition:require(success && (data.length == 0 || abi.decode(data, (bool))));The ERC20 standard define that always these two methods will return a boolean value, and it's possible to bypass this call with an empty return, if this call returns empty value, it will be with bad intentions because otherwise it will implement the ERC20 interface. It's mandatory to check that the returns it's a valid boolean value.
Recommended:
require(success && (data.length == 0 || abi.decode(data, (bool))));torequire(success && abi.decode(data, (bool)));Affected source code:
Packages with vulnerabilities
npm audit:Inconsistent solidity pragmas
The source files have different solidity compiler ranges referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.
Recently solidity released a new version with major fixes fixed, the minimum required version should be 0.8.14
Lack of input checks
The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than
address(0).Affected source code:
Unsafe ERC20 calls
The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.
Reference:
approve:Unreal comment
The comment mentions that the max argument is the sum of the amounts, but the reality is that it is not forced to be so and it can be mined faster than what is established when calling
create_lock_for.Affected source code:
Lack of ACK during owner change
It's possible to lose the ownership under specific circumstances.
Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.
Affected source code:
Possible unsync Gauge
The gauge could go out of sync if you don't add:
It could require in
addRewardTokenthe lineIGauge(gauge).addBribeRewardToken(token);:and also add the logic in swapOutRewardToken
Non-Critical
Reentrancy pattern
Although it has not been possible to exploit the reentrancy attack, the logic of the methods named below, make the changes of the validation flags after a method that allows reentrancy, it is convenient to modify the flags before the external calls to contracts.
the
lockmodifier prevents this, but it's a good idea to follow good practice.Move the
_safeTransferFromat the end of the method in:Affected source code:
Coding style
Rename temporary variables to know what they refer to:
Affected source code:
Open TODO
It's a bad practice to have unfinished code during a security review.
Affected source code: