The bug that codearena missed, twice

Right after joining trust__90 mentorship I was asked to do some research on Thena, an AMM exchange that combines Uniswap, Curve and OlympusDAO wizardry.

Extremely excited to start my adventure, I quickly open their website to take a look at the docs. It didn’t take long for me to realize I was fucked. Absolutely no docs, huge ~6000SLOC codebase and project not (fully) deployed yet.

I knew it would take me ages to make sense of what the code does. I decided to take no shortcuts and I immediately start reading the contracts line by line. It took me about 50 hours to get to the more interesting functions, like withdrawals.

Then I stumpled upon it.

The bug

The protocol allows users to lock the native ERC20 token, THE in this case, for an arbitrary amount of time in order to get a weekly yield reward.

Let’s suppose Alice decides to lock 1000 THE for 2 weeks, after doing that she gets back a veNFT with metadata:

• amount: 1000 * 1e18
• end: block.timestamp + 2 weeks

She lets 3 weeks pass by. Her veNFT is now expired, and she decides to claim her rewards by calling claim() on RewardsDistributor.sol:

function claim(uint _tokenId) external returns (uint) {
    uint amount = _claim(_tokenId, voting_escrow, _last_token_time);

    if (amount != 0) {
        IVotingEscrow(voting_escrow).deposit_for(_tokenId, amount);
        token_last_balance -= amount;
    }

    return amount;
}

which first calculates the THE reward owed and then locks the amount directly in the veNFT she’s trying to claim the rewards for. To do so the contracts calls the function deposit_for(_tokenId, amount) in VotingEscrow.sol:

function deposit_for(uint _tokenId, uint _value) external nonreentrant {
    LockedBalance memory _locked = locked[_tokenId];
    require(_value > 0); // dev: need non-zero value
    require(_locked.amount > 0, 'No existing lock found');
    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

    _deposit_for(_tokenId, _value, 0, _locked, DepositType.DEPOSIT_FOR_TYPE);
}

Here we have 3 requirements:

1. require(_value > 0): The amount Alice is trying to claim must be greater than 0
2. require(_locked.amount > 0: The amount of tokens already locked in the veNFT must be greater than 0
3. require(_locked.end > block.timestamp): The veNFT we are trying to claim the rewards for must not be expired

While requirement 1 & 2 would pass, requirement 3 would fail because Alice veNFT is, in fact, expired.

Disclosure

The bug leads to veNFT holders having their rewards frozen when trying to claim after expiry.

On 14/12/22, in collaboration with trust__90, we disclosed the bug to Thena via immunefi. It was classified as high severity and rewarded a bounty of $20k.

Post-Disclosure

I was happy, but a bit disappointed. I was hoping to find something more interesting as my first vulnerability, maybe some cool exploit, or an old school self-destruct. Being a codearena warden myself, I knew 50% of wardens would have caught this in an audit contest. Right?

I would go on to discover that Thena is a fork of Velodrome and shares some code with 3xcalibur, two protocols audited on codearena in 2022.

Nobody caught it.