Problem 1: Token
H-01. Permanent lock of funds in Token contract
Vulnerability Summary
publicMint is an payable function (i.e receive ether to the contract). But there is no way to take out funds from the contract leads to permanent locking of funds.
function publicMint(uint256 amount) external payable onlyNotBlacklisted{
...
}Recommended Mitigation
Add a function that allows owner to transfer funds from contract.
function claimFunds(address _to) payable external onlyOwner {
require(_to != address(0), "can't transfer to address(0)");
uint256 amt = address(this).balance;
(bool success, )payable(_to).call{value: amt}("");
require(success, "funds transfer failed");
emit FundsClaimed(_to, amt);
}
Problem 2: Staking
M-01. claimInterest violates CEI pattern makes it vulnerable for reentrancy for malicious tokens
Vulnerability Summary
claimInterest function transfers interest amt calculated from getAccruedInterest which calculates interest based on difference between timestamps.
function getAccruedInterest(address user) public view returns (uint256) {
stakeInfo memory t = stakes[user];
if(block.timestamp - t.timestamp >= 30 days){
return t.amt/2;
}
else if(block.timestamp - t.timestamp >= 7 days){
return t.amt/10;
}
else if(block.timestamp - t.timestamp >= 1 days){
return t.amt/100;
}
return 0;
}
Interest calculation is dependent on stakes[user] but the claimInterest transfers token first and then updates the timestamp.
function claimInterest() public {
...
stakingToken.transfer(msg.sender,inrest); // Interaction
stakes[msg.sender].timestamp = block.timestamp; // Effects
}
Impact
If the staking token is ERC777 or any malicious token. It leads to reentrancy in claimInterest function which can drain the contract balance.
Recommended Mitigation
Add a function that allows owner to transfer funds from contract.
function claimInterest() public {
...
stakes[msg.sender].timestamp = block.timestamp; // Effects
stakingToken.transfer(msg.sender,inrest); // Interaction
}
Inf-01. transferFrom may silently fail leads inconsistency in the state of contract
Vulnerability Summary
Stake function uses transferFrom to transfer tokens from sender to the contract. But it may fail silently with out revert which leads to inconsistency in state variables totalStaked, stakes.
function stake(uint256 amount) external {
...
// @audit transfer from may silently fail
stakingToken.transferFrom(msg.sender,address(this),amount);
totalStaked += amount;
t.amt = amount;
stakes[msg.sender] = t;
}Recommended Mitigation
transferFrom returns a boolean value. Add a check for successful transaction.
function stake(uint256 amount) external {
...
bool success = stakingToken.transferFrom(msg.sender,address(this),amount);
require(success, "transferring tokens failed");
...
}
Problem 3: Exchange
Note: Excluded frontrunning/backrunning/sandwich attacks that are basically overwhelming to think about during the contest.
M-01. Use call instead of transfer for ethereum native token transactions
Vulnerability Summary
In both of the removeLiquidity and tokenToEthSwap functions, transfer() is used for native ETH withdrawal. The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.
Impact
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
- The claimer smart contract does not implement a payable function.
- The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
- The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.
Recommended Mitigation
Use call() instead of transfer().
payable(msg.sender).call{value: ethAmt}("");
M-02. Accumulated ETH fees on swaps cannot be retrieved.
Vulnerability Summary
ETH fees accumulated from tokenToEthSwap() operation are permanently frozen within the contract as there is no way designed to retrieve them.
Impact
A permanent freeze of ETH revenue of the project.
Recommended Mitigation
In addition to below recommendation, ensure owner doesn't pull excess ether other than collected fee.
/// @dev used for rescuing swap fees paid to the contract in ETH
function rescueETH(address destination) external payable onlyOwner {
(bool sent, ) = destination.call{value: feevalue}('');
require(sent, 'failed');
}
Inf-01. transferFrom may silently fail
Vulnerability Summary
Stake function uses transferFrom to transfer tokens from sender to the contract. But it may fail silently with out revert.
Recommended Mitigation
transferFrom returns a boolean value. Implement a check for successful transaction.
bool success = stakingToken.transferFrom(msg.sender,address(this),amount);
require(success, "transferring tokens failed");
Inf-02. removeLiquidity violates CEI pattern leads to read-only reenrtancy
Vulnerability Summary
Implementation of removeLiquidity does burning of LPTokens after transferring the tokens and ether to the caller.
function removeLiquidity(uint256 amountOfLPTokens) external {
...
token.transfer(msg.sender,v4);
payable(msg.sender).transfer(v5);
_burn(msg.sender, amountOfLPTokens);
...
}As stated in Exchange::Inf-01 if the code is modified to use call instead of transfer, it allows caller contract to make a call to other functions.
Attack Scenario:
- Assuming attacker provided 1 ether and 10 tokens as initial liquidity. He receives 1 ether worth lpTokens i.e., 10^18 lp tokens.
- Attacker calls
removeLiquidityto remove 10^18 lp tokens. - As per implementation contract transfers 1 ether and 10 tokens back to attacker.
- Since the
_burnis being called after external call. The state oftotalSupply()isn't modified. - Attacker executes
addLiquidityinfallback
uint y = (totalSupply() * msg.value) /(ethBalance-msg.value);
_mint(msg.sender,y);As totalSupply() is higher value, attacker receives more lp tokens than expected.
Recommended Mitigation
burn LP tokens prior external calls
function removeLiquidity(uint256 amountOfLPTokens) external {
...
// burn lp tokens
_burn(msg.sender, amountOfLPTokens);
// then transfer ERC20 and Ether
token.transfer(msg.sender,v4 );
payable(msg.sender).transfer(v5);
}
Thankyou everyone participated in contest 10 and a special mention and congrats to NitinSinha 🥇
& 🤓 Get ready for the upcoming contest!