Backd contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L112-L119

Vulnerability details

Impact

I believe this to be a high severity vulnerability that is potentially included in the currently deployed StakerVault.sol contract also. The team will be contacted immediately following the submission of this report.

In StakerVault.sol, the user checkpoints occur AFTER the balances are updated in the transfer() function. The user checkpoints update the amount of rewards claimable by the user. Since their rewards will be updated after transfer, a user can send funds between their own accounts and repeatedly claim maximum rewards since the pool's inception.

In every actionable function except transfer() of StakerVault.sol, a call to ILpGauge(lpGauge).userCheckpoint() is correctly made BEFORE the action effects.

Proof of Concept

Assume a certain period of time has passed since the pool's inception. For easy accounting, assume poolStakedIntegral of LpGauge.sol equals 1. The poolStakedIntegral is used to keep track of the current reward rate.

Steps:

• Account A stakes 1000 LP tokens. balances[A] += 1000
• In the same stakeFor() function, userCheckpoint() was already called so A will already have perUserShare[A] set correctly based on their previously 0 balance and the current poolStakedIntegral.
• Account A can immediately send all balance to Account B via transfer().
• Since the checkpoint occurs after the transfer, B's balance will increase and then perUserShare[B] will be updated. The calculation for perUserShare looks as follows.

perUserShare[user] += (
            (stakerVault.stakedAndActionLockedBalanceOf(user)).scaledMul(
                (poolStakedIntegral_ - perUserStakedIntegral[user])
            )
        );

Assuming Account B is new to the protocol, their perUserStakedIntegral[user] will default to 0.

perUserShare[B] += 1000 * (1 - 0) = 1000

• B is able to call claimRewards() and mint all 1000 reward tokens.
• B then calls transfer() and sends all 1000 staked tokens to Account C.
• Same calculation occurs, and C can claim all 1000 reward tokens.
• This process can be repeated until the contract is drained of reward tokens.

Tools Used

Static review.

Recommended Mitigation Steps

In StakerVault.transfer(), move the call to ILpGauge(lpGauge).userCheckpoint() to before the balances are updated.

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkOracleProvider.sol#L55-L58

Vulnerability details

Impact

Calls to the Chainlink price oracle via getPriceUSD() in ChainlinkOracleProvider.sol use the correct function latestRoundData() per Chainlink's documentation, but lack the recommended validations to ensure that the round is complete and does not return stale data. In addition, the current implementation checks that answer >= 0, which is inclusive of a 0 price return.

Per the following Halborn audit, page 19, the recommended implementation is:

(
 roundId,
 rawPrice,
 ,
 updateTime,
 answeredInRound
) = AggregatorV3Interface(XXXXX).latestRoundData();
require(rawPrice > 0 , "Chainlink price <= 0");
require(updateTime != 0 , "Incomplete round");
require(answeredInRound >= roundId , "Stale price");

The current Backd implementation performs a validation for data freshness via require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE); It is also recommended to add a check that answeredInRound >= roundId to ensure the answer was computed in the latest round. Finally, the answer should not be allowed to equal 0, as this would allow for the unlimited purchase/minting of tokens.

Halborn audit, page 19:

https://3405344147-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2F6bWsvjSvuHlmjaYdDGxA%2Fuploads%2FxvaQXQq7NxRcRQBiGL3J%2FRolla_Finance_Quant_Protocol_Smart_Contract_Security_Audit_Report.pdf?alt=media&token=1d59da93-2e5c-4e53-9de8-a4bb6dba138e

Tools Used

Audit reports, manual review.

Recommended Mitigation Steps

Firstly, change require(answer >= 0, Error.NEGATIVE_PRICE); to be exclusive of 0. A zero price would be detrimental to the protocol.

Next, adding the require(answeredInRound >= roundId , "Stale price"); check will add extra validation that the data is not stale.

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/LiquidityPool.sol#L790-L792

Vulnerability details

Impact

The _updateUserFeesOnDeposit() function in LiquidityPool.sol is used to update a user's withdrawal fees after an action such as deposit, transfer in, etc. The withdrawal fee decays toward a minimum withdrawal fee over a period of 1 or 2 weeks (discussed with developer). Since anyone can transfer lp tokens to any user, a griefer can transfer 1 wei of lp tokens to another user to reset their lastActionTimestamp used in the withdrawal fee calculation.

The developers nicely weight the updated withdrawal fee by taking the original balance/original fee vs the added balance/added fee. The attacker will only be able to extend the runway of the withdrawal fee cooldown by resetting the lastActionTimestamp for future calculations. Example below:

Proof of Concept

Assumptions:

• MinWithdrawalFee = 0% //For easy math
• MaxWithdrawalFee = 10%
• timeToWait = 2 weeks

Steps

• User A has 100 wei of shares
• User A waits 1 week (Current withdrawal fee = 5%)
• User B deposits, receives 1 wei of shares, current withdrawal fee = 10%
• User B immediately transfers 1 wei of shares to User A

Based on the formula to calculated User A's new feeRatio:

        uint256 newFeeRatio = shareExisting.scaledMul(newCurrentFeeRatio) +
            shareAdded.scaledMul(feeOnDeposit);

In reality, User A's withdrawal fee will only increase by a negligible amount since the shares added were very small in proportion to the original shares. We can assume user A's current withdrawal fee is still 5%.

The issue is that the function then reset's User A's lastActionTimestamp to the current time. This means that User A will have to wait the maximum 2 weeks for the withdrawal fee to reduce from 5% to 0%. Effectively the cooldown runway is the same length as the original runway length, so the decay down to 0% will take twice as long.

meta.lastActionTimestamp = uint64(_getTime());

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of resetting lastActionTimestamp to the current time, scale it the same way the feeRatio is scaled. I understand that this would technically not be the timestamp of the last action, so the variable would probably need to be renamed.

Issue #1 (Low) - Setter function should check for Zero Address

When setting the community reserve address, should check for an input 0 address.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/ConvexStrategyBase.sol#L182

Issue #2 (Low) - Use of deprecated safeApprove()

OpenZeppelin lists this function as deprecated. It is recommended to use safeIncreaseAllowance.

OpenZeppelin Documentation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fcf35e5722847f5eadaaee052968a8a54d03622a/contracts/token/ERC20/utils/SafeERC20.sol#L39-L45

Link to code: https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L71

Issue #3 (Low) - Front-runnable initializers

Lack of access control on an initialize function can be front-run, leading to redeployment.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/LpToken.sol#L28-L39

Issue #4 (Low) - No transfer ownership pattern

In a couple places, transfer of an important admin address occurs without validation that the receiving address is correct. It is recommended to set the desired new address as pending until the new address is able to confirm the change. If the admin/strategist addresses are transferred incorrectly, issues will follow.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/ConvexStrategyBase.sol#L261

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/VestedEscrow.sol#L69-L72

Issue #5 (Low) - Some tokens need to approve 0 when changing from non-zero to non-zero allowance

USDT for example will not work with the current implementation. The initial approve will succeed, but subsequent approvals will fail without resetting the approval to 0 first.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L50

Issue #6 (Low) - Require message seems incorrect

The require checks the access of the sender to call the function, but the require message indicates that not enough BKD has been staked.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L546

Issue #7 (Low) Register() function will succeed with 0 value maxFee

If the user calls register() with a maxFee = 0 then the register action will fail while they send 0 ether. The action won't be able to be enacted upon, so it's simply creating waste within the protocol. This action will show up in other function calls needlessly.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L220-L222

Issue #8 (Low) - Fee on transfer tokens not supported

If a token with fee on transfer is used, these actions will fail.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/VaultReserve.sol#L59

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L340

Issue #9 (Non-critical) - Order of params in comments doesn't match implementation

depositAmount and protocol are switched.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L199-L200

Issue #10 (Non-critical) - Typo in word "Community"

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/ConvexStrategyBase.sol#L175

Issue #11 (Non-critical) - Typo in word "Note"

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L31

Issue #1 - Adding require that currentBalance > 0 earlier in function will save gas

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/GasBank.sol#L47-L49

Issue #2 - Multiple requires requires less gas than &&

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L676

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/ConvexStrategyBase.sol#L273-L276