howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingToken.sol#L125 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingToken.sol#L108

Vulnerability details

Impact

The protocol aims to work with all sorts of ERC20 tokens including tokens that charge a fee on transfer including PAXG. Deposits and withdrawals of these tokens when being used as staking tokens in StakingToken.sol are not properly handle which can cause a number of accounting issues most especially transfer of the losses due to fees to the last unstaker or reward claimer. The same effect, although to a less degree can be observed with tokens like stETH which has the 1 wei corner case issue in which the token amount transferred can be a bit less than the amount entered in. This also includes other rebasing tokens, and tokens with variable balances or airdrops as the direction of rebasing can lead to the same effect as FOT tokens, which token amount being less than internal accounting suggests, same as extra tokens from positive rebasing and airdrops being lost due it also not matching internal accounting and no other way to claim these tokens.

Proof of Concept

From the readme,

ERC20 token behaviors in scope Fee on transfer Yes Balance changes outside of transfers Yes

And looking at StakingToken.sol, we can see that the amount of the stakingToken is directly transferred as is and is also recorded into the contract's balance and avaliableRewards. Now if stakingToken is a token like PAXG that charges a fee on transfer, the actual amount transferred from msg.sender upon deposit will be different from amount received by StakingToken.sol and recorded into the balance/availableRewards. Here, there becomes a mismatch between the contract's accounting and the contract's actual balance.

    function deposit(uint256 amount) external {
        // Add to the contract and available rewards balances
        uint256 newBalance = balance + amount;
        uint256 newAvailableRewards = availableRewards + amount;

        // Record the new actual balance and available rewards
        balance = newBalance;
        availableRewards = newAvailableRewards;

        // Add to the overall balance
        SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);

        emit Deposit(msg.sender, amount, newBalance, newAvailableRewards);
    }

The same can be seen in _withdraw which is called when a user claims or unstakes. The amount the user receives will be less that he expects while the wrong amount is emitted to him.

    function _withdraw(address to, uint256 amount) internal override {
        // Update the contract balance
        balance -= amount;

        SafeTransferLib.safeTransfer(stakingToken, to, amount);

        emit Withdraw(to, amount);
    }

The ultimate effect will be seen during user claims and unstake in which a point will be reached where the stakingToken balance in the contract will be zero, while the internal accounting still registers that there are still rewards available for users to claim. In an attempt to rescue the situation, the protcol will incur extra costs in sending tokens to the StakingToken contract address to momentarily balance out the the token balance and the internal amount tracking. This same effect can be observed in rebasing tokens when they rebase negatively, or during the 1 wei corner case as actual available balance is less that accounting balance. Funds will also be lost if the contract incurs extra airdrops from the stakingToken or it positively rebases.

Tools Used

Manual code review

Recommended Mitigation Steps

Query token balance before and after transfers or disable support for these token types. A skim function can also be introduced to remove any excess tokens from positive rebases.