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

Unchecked `bool` return values for Solmate's ERC20 - `transferFrom` implementation, can end up with a malicious actor stealing funds from the staking contract.

dimah7

Summary

The transfer and transferFrom functions from Solmates's ERC20 contract return a bool value, indicating whether a transfer is successful or not. However the issue with this is that, if the transfer fails and it returns false, the transaction doesn't revert. If there are some state changes made during the execution of the transaction, this can leave a protocol open for exploits. Checking the return value is a requirement as per EIP-20 standard's documentation:

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Neither one of protocol's functions (Airdrop::claim, Staking::deposit, Staking::withdraw, Staking::claimRewards) that use transfer or transferFrom, doesn't handle the returned value. This can lead to a malicious user draining all the funds from the Staking contract.

Vulnerability Details

I made a LoveTokenMock contract for this one, which is the same like LoveToken.sol, just added thеse two functions for simplicity and facilitation of the Proof-Of-Code:

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

Add the following in StakingTest.t.sol file, here is a scenario:

  1. Let's say some users already have staked their love tokens to the Staking contract, so it will start with 10 ether initial balance.

  2. The malicious actor first deposits the transfer fails, but the userStakes mapping needs to be updated. Because when withdrawing it updates it again, subtracting the specified amount from the mapping. Otherwise if it's with default value it will revert with arithmetic underflow error.

  3. Finally withdraws specifying the Staking contract's balance as a parameter.

  4. Ends up stealing all the tokens.

Code here:
function testStealFromStakingContract() public {
address maliciousUser = makeAddr("malicious user");
​
assert(loveToken.balanceOf(maliciousUser) == 0);
uint256 stakingInitialBalance = 10e18;
loveToken.mint(address(stakingContract), stakingInitialBalance);
assert(loveToken.balanceOf(address(stakingContract)) == stakingInitialBalance);
​
vm.startPrank(maliciousUser);
stakingContract.deposit(stakingInitialBalance);
assert(stakingContract.userStakes(maliciousUser) == stakingInitialBalance);
​
stakingContract.withdraw(stakingInitialBalance);
vm.stopPrank();
​
assert(loveToken.balanceOf(address(stakingContract)) == 0);
assert(loveToken.balanceOf(maliciousUser) == stakingInitialBalance);
}

Impact

Likelihood is Low/Medium to happen, severity is High because funds are at risk -> impact: Medium

Tools Used

Manual Review, Foundry

Recommendations

  1. Consider implementing OpenZeppelin's SafeERC20 library for transfer functions.

  2. Or do a check with custom error for to the returned bool value.

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.