Venus Prime - seerether's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L820-L821 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L221 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L828

Vulnerability details

Impact

• This can allow pendingScoreUpdates to reach 0 and finalize the round, even though not all tokens were updated.
• This could allow an attacker to manipulate pendingScoreUpdates to bypass the score update limit.

Proof of Concept

Burning during a score update round can leave pendingScoreUpdates incorrect. Can invalidate assumptions. Should prevent burning during round.

1. The Prime contract keeps track of pendingScoreUpdates, which is the number of user scores left to update in the current round. This is initialized to the total number of token holders: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L820-L821

2. When a user's score is updated in the round, pendingScoreUpdates is decremented: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L221

3. However, if a token is burned during the round, it decrements totalScoreUpdatesRequired but does not adjust pendingScoreUpdates: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L828

This means pendingScoreUpdates could end up higher than totalScoreUpdatesRequired, invalidating the assumption that it tracks tokens left to update. For example, say there are 100 tokens initially, so pendingScoreUpdates is initialized to 100. 50 tokens are updated, so pendingScoreUpdates = 50. Now a token is burned, so totalScoreUpdatesRequired decreases to 99, but pendingScoreUpdates is still 50.

Tools Used

Manual

Recommended Mitigation Steps

Burning tokens should be disabled during a score update round. pendingScoreUpdates should be recalculated if a token is burned. Something like:

// Disable burning if round active require(nextScoreUpdateRoundId == 0, "Burning not allowed during round");

// Adjust pendingScoreUpdates if burned _updateRoundAfterTokenBurned(user);

This will help ensure pendingScoreUpdates stays valid even if tokens are burned.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L216-L220

Vulnerability details

Impact

This can allow the owner to sweep more tokens than are available

Proof of Concept

The sweepToken() function allows the owner to transfer any amount of a token to a recipient address. It only checks that the amount is less than the token's balanceOf the contract: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L216-L220 However, it does not take into account the tokenAmountAccrued mapping, which tracks accrued but not yet transferred amounts for each token. For example: • Contract balance of TokenA: 100 • tokenAmountAccrued of TokenA: 50 • Actual available balance: 100 - 50 = 50 The owner could call sweepToken(TokenA, owner, 80), which would succeed since 80 < 100 balanceOf contract. But in reality only 50 tokens are available.

In more details, Here is how an owner could exploit this to sweep more tokens than available:

1. Assume token X has 100 tokens deposited to the contract.
2. The owner sets a distribution speed of 10 tokens per block for token X.
3. Over 10 blocks, 100 tokens will accrue to the tokenAmountAccrued[tokenX] mapping.
4. The accrued balance is 100 tokens, but the actual contract balance for token X is still 100 tokens.
5. The owner calls sweepToken(tokenX, owner, 150) to sweep 150 tokens.
6. The sweepToken function checks tokenX.balanceOf(contract) which is 100 tokens.
7. But it does not check the tokenAmountAccrued[tokenX] which is 100 tokens.
8. So the actual available balance is 100 - 100 = 0 tokens.
9. But sweepToken thinks the available balance is 100 tokens based on the balanceOf() call.
10. So it will allow the sweep of 150 tokens even though only 100 tokens are available.
11. This means 50 excess tokens get swept to the owner account.

To summarize, the vulnerability is that sweepToken relies only on the token contract's balanceOf() instead of checking the accrued but not transferred balance. An owner could exploit this to sweep more tokens than are available in the contract.

Tools Used

Manual

Recommended Mitigation Steps

The sweepToken function should also subtract the accrued but not yet transferred amount for that token:

uint256 availableBalance = token_.balanceOf(address(this)) - tokenAmountAccrued[token_]; if (amount_ > availableBalance) { revert InsufficientBalance(amount_, availableBalance);
}

This would properly account for both the actual balance and the accrued amount, preventing the owner from sweeping more than is available.

Assessed type

Other