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

Missing check of returned values of some functions

bube

Summary

The returned values of functions like transfer, transferFrom and approve are not checked.

Vulnerability Details

The functions Staking::deposit, Staking::withdraw, Staking::claimRewards and Airdrop::claim call transfer and transferFrom in order to transfer tokens from/to msg.sender, stakingContract, stakingVault. But it is not checked if these functions are correclty executed:

`Staking`:
​
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);
}
​
/// @notice Decrease the userStakes variable and transfer LoveToken to the user withdrawing.
function withdraw(uint256 amount) public {
// No require needed because of overflow protection
userStakes[msg.sender] -= amount;
@> loveToken.transfer(msg.sender, amount);
emit Withdrew(msg.sender, amount);
}
​
/// @notice Claim rewards for staking.
/// @notice Users can claim 1 token per staking token per week.
function claimRewards() public {
uint256 soulmateId = soulmateContract.ownerToId(msg.sender);
// first claim
if (lastClaim[msg.sender] == 0) {
lastClaim[msg.sender] = soulmateContract.idToCreationTimestamp(
soulmateId
);
}
​
// How many weeks passed since the last claim.
// Thanks to round-down division, it will be the lower amount possible until a week has completly pass.
uint256 timeInWeeksSinceLastClaim = ((block.timestamp -
lastClaim[msg.sender]) / 1 weeks);
​
if (timeInWeeksSinceLastClaim < 1)
revert Staking__StakingPeriodTooShort();
​
lastClaim[msg.sender] = block.timestamp;
​
// Send the same amount of LoveToken as the week waited times the number of token staked
uint256 amountToClaim = userStakes[msg.sender] *
timeInWeeksSinceLastClaim;
@> loveToken.transferFrom(
address(stakingVault),
msg.sender,
amountToClaim
);
​
emit RewardsClaimed(msg.sender, amountToClaim);
}
​
`Airdrop`:
​
function claim() public {
// No LoveToken for people who don't love their soulmates anymore.
if (soulmateContract.isDivorced()) revert Airdrop__CoupleIsDivorced();
​
// Calculating since how long soulmates are reunited
uint256 numberOfDaysInCouple = (block.timestamp -
soulmateContract.idToCreationTimestamp(
soulmateContract.ownerToId(msg.sender)
)) / daysInSecond;
​
uint256 amountAlreadyClaimed = _claimedBy[msg.sender];
​
if (
amountAlreadyClaimed >=
numberOfDaysInCouple * 10 ** loveToken.decimals()
) revert Airdrop__PreviousTokenAlreadyClaimed();
​
uint256 tokenAmountToDistribute = (numberOfDaysInCouple *
10 ** loveToken.decimals()) - amountAlreadyClaimed;
​
// Dust collector
if (
tokenAmountToDistribute >=
loveToken.balanceOf(address(airdropVault))
) {
tokenAmountToDistribute = loveToken.balanceOf(
address(airdropVault)
);
}
_claimedBy[msg.sender] += tokenAmountToDistribute;
​
emit TokenClaimed(msg.sender, tokenAmountToDistribute);
​
@> loveToken.transferFrom(
address(airdropVault),
msg.sender,
tokenAmountToDistribute
);
}
​

Additionally, in the LoveToken::initVault is used approve function to approve amount of tokens. But the returned value from this function is not checked. If the approvement is not successful, the protocol would not work as intended.

Impact

If some of the stakingVault, stakingContract or msg.sender doesn't have enough balance, the transaction will revert with due Reason: panic: arithmetic underflow or overflow. But the user may not uderstand why this is happened.
And if the LoveToken::initVault doesn't succeed to initiate properly the Staking and Airdrop contracts, the functionality of the protocol will be broken.
If the approve function fails and does not return true, it means that the allowance was not set correctly. Since the return value of this function is not checked in the LoveToken::initVault constructor, the contract deployment will proceed as if the allowance was set correctly. This could lead to serious issues later on when the Staking and Airdrop contracts try to transfer tokens from the stakingVault and airdropVault. If the allowance was not set correctly, these transfer operations will fail, but the contract has no way of knowing this in advance because the return value of the approve function was not checked.

Tools Used

Manual Review

Recommendations

Add a check with meaningfull error message in the functions Staking::deposit, Staking::withdraw, Staking::claimRewards to ensure that the contract that transfer tokens has enough amount and check the returned value from the transfer and transferFrom functions.
Also, check of the return value of the transfer and transferFrom functions is required in Airdrop::claim function and in Lovetoken::initVault for the approve function.

Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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

Give us feedback!