Back to reassor's contests

Badger-Vested-Aura contest - reassor's results

Bringing BTC to DeFi

General Information

Platform: Code4rena

Start Date: 15/06/2022

Pot Size: $30,000 USDC

Total HM: 5

Participants: 55

Period: 3 days

Judge: Jack the Pug

Id: 138

League: ETH

BadgerDAO

Findings Distribution

Researcher Performance

Rank: 18/55

Findings: 2

Award: $188.08

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryBadgerDAO

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xFar5eer, 0xNazgul, 0xNineDec, 242, Chom, Czar102, Funen, GimelSec, Meera, Picodes, Sm4rty, Tadashi, TerrierLover, Waze, _Adam, a12jmx, asutorufos, codexploder, cryptphi, defsec, gzeon, hyh, joestakey, minhquanym, oyc_109, reassor, robee, saian, sorrynotsorry, unforgiven, zzzitron

Awards

102.4813 USDC - $102.48

Labels

bug
QA (Quality Assurance)
sponsor acknowledged
valid

External Links

Link to GitHub issue

1. Missing zero address checks

Risk

Low

Impact

Missing checks for zero addresses might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

MyStrategy.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

2. Missing events

Risk

Low

Impact

Contract is missing emitting events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

MyStrategy.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing events to listed functions.

3. Missing indexing for events

Risk

Non-Critical

Impact

Events should index addresses which helps off-chain applications in monitoring the protocol.

Proof of Concept

MyStrategy.sol:

Tools Used

Manual Review / VSCode

It is recommended to add indexing to address type parameters.

4. Deprecated safeApprove

Risk

Non-Critical

Impact

Function safeApprove has been deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().

Proof of Concept

MyStrategy.sol:65:        AURA.safeApprove(address(LOCKER), type(uint256).max);
MyStrategy.sol:67:        AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
MyStrategy.sol:68:        WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

Tools Used

Manual Review / VSCode

It is recommended to consider using safeIncreaseAllowance() and safeDecreaseAllowance() functions instead of safeApprove.

5. Missing natspec comments

Risk

Non-Critical

Impact

Contract is missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

MyStrategy.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing natspec comments.

#0 - GalloDaSballo

2022-06-19T01:33:42Z

I'm pretty confident you don't need to natspec things when they are obvious.

As a wise programmer would say: The best comment is the one you don't need to write

#1 - lucyoa

2022-06-30T20:50:25Z

natspec will let you generate documentation easily

and since we are quoting, let me drop one too :) "Documentation is like sex: when it is good, it is very, very good; and when it is bad, it is better than nothing."

Findings Information

🌟 Selected for report: 0xKitsune

Also found by: 0x1f8b, 0xFar5eer, 0xNazgul, 0xkatana, Chom, Funen, IllIllI, JC, Meera, MiloTruck, Picodes, TerrierLover, TomJ, Waze, _Adam, c3phas, codexploder, defsec, fatherOfBlocks, gzeon, joestakey, oyc_109, reassor, rfa, robee, sach1r0, simon135

Awards

85.5954 USDC - $85.60

Labels

bug
G (Gas Optimization)
sponsor acknowledged
valid

External Links

Link to GitHub issue

1. Costly reentrancy guard writes in the loop

Impact

Contract is implemeting sweepRewardToken that is protected against reentrancy attacks with reentrancy guard. The issue is that the function sweepRewardToken can be executed in the loop through sweepRewards function which makes costly reentrancy guard changes with every iteration and invocation of sweepRewardToken.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to consider making sweepRewardToken an internal function (removing nonReentrant) and adding nonReentrant to sweepRewards that will be used also for sweeping one token with (single element in tokens array).

2. Unnecessary calculations

Impact

Function MyStrategy.claimBribesFromHiddenHand is iterating over the array of claims to handle reward transfers. First it calculates balances for ether and tokens and then uses math calculation to understand how much of those should be sent via _handleRewardTransfer. The issue with this approach is that since the contract does not allow withdrawing ether the value of address(this).balance should always be 0 (otherwise ether will get locked) thus it is better just to use address(this).balance without spending gas on calculating the difference.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to skip calculations for ether difference and always pass to _handleRewardTransfer value of all ether in the contract - address(this).balance.

3. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

MyStrategy.sol:300:        for (uint256 i = 0; i < _claims.length; i++) {
MyStrategy.sol:317:        for (uint256 i = 0; i < _claims.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to cache the array length outside of for loop.

4. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

MyStrategy.sol:184:         require(
MyStrategy.sol:185:             balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
MyStrategy.sol:186:             "You have to wait for unlock or have to manually rebalance out of it"
MyStrategy.sol:187:         );

Tools Used

Manual Review / VSCode

It is recommended to decrease revert messages to maximum 32 bytes.

5. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

118:        for(uint i = 0; i < length; i++){
300:        for (uint256 i = 0; i < _claims.length; i++) {
317:        for (uint256 i = 0; i < _claims.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

MyStrategy.sol:118:        for(uint i = 0; i < length; i++){
MyStrategy.sol:300:        for (uint256 i = 0; i < _claims.length; i++) {
MyStrategy.sol:317:        for (uint256 i = 0; i < _claims.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to remove explicit initializations with default values.

7. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

MyStrategy.sol:230:        if (auraBalEarned > 0) {
MyStrategy.sol:279:        if (harvested[0].amount > 0) {
MyStrategy.sol:323:                if (difference > 0) {
MyStrategy.sol:330:                if (difference > 0) {
MyStrategy.sol:387:        upkeepNeeded = unlockable > 0;

Tools Used

Manual Review / VSCode

It is recommended to use != 0 instead of > 0.

#0 - GalloDaSballo

2022-06-19T01:32:45Z

1. Costly reentrancy guard writes in the loop

Probably the highest gas savings

rest I ack

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter