Submission Details
Severity:
The return values for transfer/transferFrom are not checked in FjordStaking contract
Summary
As a best practice, the return values for transfer/transferFrom function should be checked to ensure that the call succeeded or failed.
The implementation of FjordStaking contract does not check the return values for transfer/transferFrom functions.
Vulnerability Details
function stakeVested(uint256 _streamID) external checkEpochRollover redeemPendingRewards {
//CHECK
if (!sablier.isStream(_streamID)) revert NotAStream();
if (sablier.isCold(_streamID)) revert NotAWarmStream();
// only allow authorized stream sender to stake cancelable stream
if (!authorizedSablierSenders[sablier.getSender(_streamID)]) {
revert StreamNotSupported();
}
if (address(sablier.getAsset(_streamID)) != address(fjordToken)) revert InvalidAsset();
uint128 depositedAmount = sablier.getDepositedAmount(_streamID);
uint128 withdrawnAmount = sablier.getWithdrawnAmount(_streamID);
uint128 refundedAmount = sablier.getRefundedAmount(_streamID);
if (depositedAmount - (withdrawnAmount + refundedAmount) <= 0) revert InvalidAmount();
uint256 _amount = depositedAmount - (withdrawnAmount + refundedAmount);
//EFFECT
userData[msg.sender].unredeemedEpoch = currentEpoch;
DepositReceipt storage dr = deposits[msg.sender][currentEpoch];
if (dr.epoch == 0) {
dr.vestedStaked = _amount;
dr.epoch = currentEpoch;
_activeDeposits[msg.sender].add(currentEpoch);
} else {
dr.vestedStaked += _amount;
}
_streamIDs[msg.sender][_streamID] = NFTData({ epoch: currentEpoch, amount: _amount });
_streamIDOwners[_streamID] = msg.sender;
newStaked += _amount;
newVestedStaked += _amount;
//INTERACT
--> sablier.transferFrom({ from: msg.sender, to: address(this), tokenId: _streamID });
points.onStaked(msg.sender, _amount);
emit VestedStaked(msg.sender, currentEpoch, _streamID, _amount);
}
function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
NFTData storage data = _streamIDs[streamOwner][_streamID];
DepositReceipt storage dr = deposits[streamOwner][data.epoch];
if (amount > data.amount) revert InvalidAmount();
bool isFullUnstaked = data.amount == amount;
uint16 epoch = data.epoch;
dr.vestedStaked -= amount;
if (currentEpoch != data.epoch) {
totalStaked -= amount;
totalVestedStaked -= amount;
userData[streamOwner].totalStaked -= amount;
} else {
// unstake immediately
newStaked -= amount;
newVestedStaked -= amount;
}
if (dr.vestedStaked == 0 && dr.staked == 0) {
// instant unstake
if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
userData[streamOwner].unredeemedEpoch = 0;
}
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
// fully unstake
if (isFullUnstaked) {
delete _streamIDs[streamOwner][_streamID];
delete _streamIDOwners[_streamID];
} else {
data.amount -= amount;
}
//INTERACT
if (isFullUnstaked) {
--> sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
points.onUnstaked(msg.sender, amount);
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}
function unstakeAll()
external
checkEpochRollover
redeemPendingRewards
returns (uint256 totalStakedAmount)
{
uint256[] memory activeDeposits = getActiveDeposits(msg.sender);
if (activeDeposits.length == 0) revert NoActiveDeposit();
for (uint16 i = 0; i < activeDeposits.length; i++) {
uint16 epoch = uint16(activeDeposits[i]);
DepositReceipt storage dr = deposits[msg.sender][epoch];
if (dr.epoch == 0 || currentEpoch - epoch <= lockCycle) continue;
totalStakedAmount += dr.staked;
// no vested staked and stake is 0 then delete the deposit
if (dr.vestedStaked == 0) {
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
} else {
// still have vested staked, then only delete the staked
dr.staked = 0;
}
}
totalStaked -= totalStakedAmount;
userData[msg.sender].totalStaked -= totalStakedAmount;
--> fjordToken.transfer(msg.sender, totalStakedAmount);
points.onUnstaked(msg.sender, totalStakedAmount);
// emit event
emit UnstakedAll(
msg.sender, totalStakedAmount, activeDeposits, getActiveDeposits(msg.sender)
);
}
Impact
There is a potential that the internal account will break if the transfer() or transferFrom() actually fails, but since the return value is being ignored, the calling contract assumes that it will always succeed. This will break the internal accounting for the calling contract.
Tools Used
Manual review
Recommendations
Always check the return values for the transfer() and transferFrom() functions. Some ERC tokens does not follow the standard, hence it is recommended to use safeTransfer functions instead.
Prize pool breakdown
Total prize
20,000 USDC
nSLOC
66230 USDC / LOC
16,000 USDC
2,500 USDC
1,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.