code-423n4 / 2024-05-olas-findings

13 stars 4 forks source link

StakingBase.sol can’t handle fee-on-transfer token #81

Closed howlbot-integration[bot] closed 4 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Vulnerability Detail

Olas Protocol allows to utilise “fee-on-transfer” (PAXG, STA) tokens as stakingToken. However, the deposit function isn’t implemented in such way, so it could handle the “fee-on-transfer” token.

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;
        //@audit
        // Add to the overall balance
        SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);

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

Proof of Concept

  1. Assume user deposit 10 PAXG tokens. The newBalance is incremented with passed amount exactly, while in reality the safeTransferFrom amount will be less. Let’s assume that 9.5 PAXG tokens will be transferred. It means, that availableRewards will be incremented incorrectly
  2. When the owner of the StakingBase contract claim the reward, the rewards will be based on the total availableRewards . It means reward calculation will be based on the 10 PAXG tokens instead of actual amount transferred which is 9.5 PAXG
  3. The claim function, as well as unstake will fail, because it will try to _withdraw more than the contract hold.

Impact

Owner can’t claim / unstake the rewards.

Recommendation

In deposit function , add only the actual transferred amounts (computed by PostTransferBalance - PreTransferBalance

Assessed type

ERC20

c4-judge commented 4 months ago

0xA5DF marked the issue as satisfactory