First Flight #9: Soulmate
First Flight #9
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Unhandling of returned boolean value from `transferFrom` function in `Staking::deposit` can lead to tokens theft from staking vault.

dimah7

Summary

Based on Solmate ERC20 contract's implementation. The transferFrom function returns a boolean value, indicating if a transfer proceeded or not, but it's not reverting the transaction which is an issue. Lack of successful transfer check in Staking::deposit can allow an attacker to steal rewards from staking vault.

function deposit(uint256 amount) public {
if (loveToken.balanceOf(address(stakingVault)) == 0)
revert Staking__NoMoreRewards();
// No require needed because of overflow protection
userStakes[msg.sender] += amount;
@> loveToken.transferFrom(msg.sender, address(this), amount);
​
emit Deposited(msg.sender, amount);
}

Vulnerability Details

I implemented this transferFrom function that intentionally returns false in LoveToken.sol, to conduct the test case:

function transferFrom(address /*from*/, address /*to*/, uint256 /*amount*/) public pure override returns (bool) {
return false;
}

As pointed out in the documentation of the protocol:

For every 1 token deposited and 1 week left in the contract, 1 LoveToken is rewarded.

Examples:

7 tokens deposited for 2 weeks = 14 LoveToken reward

  1. An attacker stakes 7 ether of love token the transfer fails because of the custom transfer function, but the userStakes mapping is updated, which plays a crutial role for executing the attack.

  2. The tokens are left for 2 weeks in the contract.

  3. The attacker claims his 14 love token reward.

Note on the PoC: Since the mock transferFrom function is also used in Staking::claimRewards the transfer will fail, but the accrued reward tokens can be seen in the emitted RewardsClaimed event, by making the verbosity higher level from the command line.

Add the following in StakingTest.t.sol file and run forge test --mt testStealFromStakingVault -vvvv in the command line:

function testStealFromStakingVault() public {
address attacker = makeAddr("attacker");
​
assert(loveToken.balanceOf(attacker) == 0);
assert(loveToken.balanceOf(address(stakingVault)) == 500_000_000 ether);
uint256 depositAmount = 7 ether;
vm.startPrank(attacker);
stakingContract.deposit(depositAmount);
assert(stakingContract.userStakes(attacker) == depositAmount);
vm.warp(block.timestamp + 2 weeks);
​
stakingContract.claimRewards();
vm.stopPrank();
}

Logs from the event:

[513] LoveToken::transferFrom(Vault: [0x1240FA2A84dd9157a0e76B5Cfe98B1d52268B264], attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], 14000000000000000000 [1.4e19])
│ │ └─ ← false
│ ├─ emit RewardsClaimed(user: attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], @>>> amount: 14000000000000000000 [1.4e19])
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
└─ ← ()

Impact

Medium: It may happen under specific condition, but the severity will be critical for the protocol.

Tools Used

Manual Review, Foundry

Recommendations

Use OpenZeppelin's SafeERC20 library for safe transfers or check the returned bool values of all transfer and transferFrom functions in the protocol with require or if statement.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.