Fjord Token Staking
Fjord
DeFiFoundry
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

completeClaimRequest()' is subject to reentrancy attack that can drain the token supply

Jan

Summary

The function completeClaimRequest() transfers tokens to a user but does not decrease the unclaimedRewards before making the transfer.

This can result in a re-entrancy attack because if a malicious contract receives these tokens and immediately calls completeClaimRequest() again before unclaimedRewards is updated, it can drain the token supply.

Vulnerability Details

https://github.com/Cyfrin/2024-08-fjord/blob/main/src/FjordStaking.sol#L662-L687

function completeClaimRequest() external checkEpochRollover redeemPendingRewards returns (uint256 rewardAmount) {
ClaimReceipt memory cr = claimReceipts[msg.sender];
if (cr.requestEpoch < 1) revert ClaimReceiptNotFound();
if (currentEpoch - cr.requestEpoch <= claimCycle) revert CompleteRequestTooEarly();
rewardAmount = cr.amount;
userData[msg.sender].unclaimedRewards -= rewardAmount;
totalRewards -= rewardAmount;
delete claimReceipts[msg.sender];
// Here tokens are transferred before unclaimed rewards are updated
fjordToken.safeTransfer(msg.sender, rewardAmount);
emit RewardClaimed(msg.sender, rewardAmount);
}

The contract can be potentially vulnerable to a re-entrancy attack as the state variable unclaimedRewards is updated after the external call to safeTransfer(). An attacker can create a contract that calls completeClaimRequest() in its fallback function resulting in multiple withdrawals.

contract Attacker {
FjordStaking fjordStaking;
​
constructor(FjordStaking _fjordStaking) public {
fjordStaking = _fjordStaking;
}
​
fallback() external payable {
fjordStaking.completeClaimRequest();
}
​
function attack() public {
fjordStaking.completeClaimRequest();
}
}

Impact

A malicious contrast can drain the token supply

Tools Used

Manual review

Recommendations

To protect the function from a potential re-entrancy attack, it's advisable to make sure all internal state changes are made before calling any external contracts. The state variable should be updated before the call to safeTransfer().

In the completeClaimRequest() function, reorder the statements as follows:

rewardAmount = cr.amount;
userData[msg.sender].unclaimedRewards -= rewardAmount;
totalRewards -= rewardAmount;
delete claimReceipts[msg.sender];
fjordToken.safeTransfer(msg.sender, rewardAmount); // call external contract after state updates
emit RewardClaimed(msg.sender, rewardAmount);
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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