Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/001-M
CodingNameKiki, millers.planet, 0xNazgul, cccz, Bnke0x0, Chom, Waze, IEatBabyCarrots, TomJ, Tomo, hickuphh3, pashov, sach1r0, Sm4rty, IllIllI, chainNue, Dravee
It is recommended to use safeTransferFrom()
instead of transferFrom()
when transferring ERC721s out of the vault.
The transferFrom()
method is used instead of safeTransferFrom()
, which I assume is a gas-saving measure. I however argue that this isn’t recommended because:
- OpenZeppelin’s documentation discourages the use of
transferFrom()
; usesafeTransferFrom()
whenever possible - The recipient could have logic in the
onERC721Received()
function, which is only triggered in thesafeTransferFrom()
function and not intransferFrom()
. A notable example of such contracts is the Sudoswap pair:
function onERC721Received(
address,
address,
uint256 id,
bytes memory
) public virtual returns (bytes4) {
IERC721 _nft = nft();
// If it's from the pair's NFT, add the ID to ID set
if (msg.sender == address(_nft)) {
idSet.add(id);
}
return this.onERC721Received.selector;
}
- It helps ensure that the recipient is indeed capable of handling ERC721s.
While unlikely because the recipient is the function caller, there is the potential loss of NFTs should the recipient is unable to handle the sent ERC721s.
Use safeTransferFrom()
when sending out the NFT from the vault.
- IERC721(_erc721Address).transferFrom(address(this), msg.sender, _id);
+ IERC721(_erc721Address).safeTransferFrom(address(this), msg.sender, _id);
Note that the vault would have to inherit the IERC721Receiver
contract if the change is applied to Transfer.sol
as well.
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/004-M
minhquanym, JohnSmith, IllIllI
Mistakes made on one chain can be re-applied to a new chain
There is no chain.id
in the signed data
If a user does a changeRecipientAddress()
using the wrong network, an attacker can replay the action on the correct chain, and steal the funds a-la the wintermute gnosis safe attack, where the attacker can create the same address that the user tried to, and steal the funds from there
Manual Review
Include the chain.id
in what's hashed
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/005-M
Lambda, cccz, hansfriese, IEatBabyCarrots, rbserver, JohnSmith, minhquanym, Tomo, leastwood, dipp, defsec, HonorLt, IllIllI, saian, csanuragjain
In case ERC20 token is fee-on-transfer, Vault can loss funds when users withdraw
In Transfer.transferERC20()
function, this function called logIncomingERC20()
with the exact amount used when it called safeTransferFrom()
. In case ERC20 token is fee-on-transfer, the actual amount that Vault received may be less than the amount is recorded in logIncomingERC20()
.
The result is when a user withdraws his funds from Vault
, Vault can be lost and it may make unable for later users to withdraw their funds.
Consider the scenario
- Token X is fee-on-transfer and it took 10% for each transfer. Alice has 1000 token X and Bob has 2000 token X
- Assume that both Alice and Bob are attacked. Harpie transfers all token of Alice and Bob to Vault. It recorded that the amount stored for token X of Alice is 1000 and Bob is 2000. But since token X has 10% fee, Vault only receives 2700 token X.
- Now Bob withdraw his funds back. With
amountStored = 2000
, he will transfer 2000 token X out of the Vault and received 1800. - Now the Vault only has 700 token X left and obviously it's unable for Alice to withdraw
Manual Review
Consider calculating the actual amount Vault received to call logIncomingERC20()
Transfer the tokens first and compare pre-/after token balances to compute the actual transferred amount.
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/007-M
Lambda, cccz, yixxas, Waze, IEatBabyCarrots, pashov, 0xSmartContract, JohnSmith, Tomo, CodingNameKiki, sach1r0, IllIllI, csanuragjain, gogo
The function withdrawPayments() is used by the Owners to withdraw the fees.
transfer() uses a fixed amount of gas, which was used to prevent reentrancy. However this limit your protocol to interact with others contracts that need more than that to process the transaction.
Specifically, the withdrawal will inevitably fail when: 1.The withdrawer smart contract does not implement a payable fallback function. 2.The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units. 3.The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.
transfer() uses a fixed amount of gas, which can result in revert. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
https://github.com/Harpieio/contracts/blob/97083d7ce8ae9d85e29a139b1e981464ff92b89e/contracts/Vault.sol#L159 https://github.com/Harpieio/contracts/blob/97083d7ce8ae9d85e29a139b1e981464ff92b89e/contracts/Vault.sol#L156-L160
Manual Review
Use call instead of transfer(). Example: (bool succeeded, ) = _to.call{value: _amount}(""); require(succeeded, "Transfer failed.");
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/008-M
hickuphh3, 0xSmartContract, xiaoming90, ak1, minhquanym, leastwood, defsec, HonorLt
https://github.com/Harpieio/contracts/blob/97083d7ce8ae9d85e29a139b1e981464ff92b89e/contracts/Transfer.sol#L57 https://github.com/Harpieio/contracts/blob/97083d7ce8ae9d85e29a139b1e981464ff92b89e/contracts/Transfer.sol#L88
There is no upper limit on the amount of fee users have to pay to withdraw their funds back. So any EOA can call transfer function on Transfer
contract can set an unreasonable amount of fee and users have to pay it if they want their funds back. We need to make sure that users' funds cannot be loss even when the protocol acts maliciously.
In case the protocol acts maliciously and set fee = 1e18
to transfer users' fund to Vault
, users cannot withdraw their funds since fee is too high.
In both transferERC20()
and transferERC721()
, EOA is caller and can set fee
param to any value it wants.
function transferERC721(address _ownerAddress, address _erc721Address, uint256 _erc721Id, uint128 _fee) public returns (bool) {
require(_transferEOAs[msg.sender] == true || msg.sender == address(this), "Caller must be an approved caller.");
require(_erc721Address != address(this));
(bool transferSuccess, bytes memory transferResult) = address(_erc721Address).call(
abi.encodeCall(IERC721(_erc721Address).transferFrom, (_ownerAddress, vaultAddress, _erc721Id))
);
require(transferSuccess, string (transferResult));
(bool loggingSuccess, bytes memory loggingResult) = address(vaultAddress).call(
abi.encodeCall(Vault.logIncomingERC721, (_ownerAddress, _erc721Address, _erc721Id, _fee))
);
require(loggingSuccess, string (loggingResult));
emit successfulERC721Transfer(_ownerAddress, _erc721Address, _erc721Id);
return transferSuccess;
}
And users need to send enough fee (native token) to withdraw their fund back on Vault
function withdrawERC721(address _originalAddress, address _erc721Address, uint256 _id) payable external {
require(_recipientAddress[_originalAddress] == msg.sender, "Function caller is not an authorized recipientAddress.");
require(_erc721Address != address(this), "The vault is not a token address");
require(canWithdrawERC721(_originalAddress, _erc721Address, _id), "Insufficient withdrawal allowance.");
require(msg.value >= _erc721WithdrawalAllowances[_originalAddress][_erc721Address][_id].fee, "Insufficient payment.");
_erc721WithdrawalAllowances[_originalAddress][_erc721Address][_id].isStored = false;
_erc721WithdrawalAllowances[_originalAddress][_erc721Address][_id].fee = 0;
IERC721(_erc721Address).transferFrom(address(this), msg.sender, _id);
}
Manual Review
Consider adding an upper limit on the amount of fee users need to pay
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/010-M
0xNazgul, pashov, IllIllI, ladboy233, defsec, sirhashalot
OpenZeppelin has a vulnerability in versions lower than 4.7.3, which can be exploited by an attacker. The project uses a vulnerable version
All of the conditions from the advisory are satisfied: the signature comes in a single bytes
argument, ECDSA.recover()
is used, and the signatures themselves are used for replay protection checks
https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-4h98-2769-gh6h
If a user calls changeRecipientAddress()
, notices a mistake, then calls changeRecipientAddress()
again, an attacker can use signature malleability to re-submit the first change request, as long as the old request has not expired yet.
The wrong, potentially now-malicious, address will be the valid change recipient, which could lead to the loss of funds (e.g. the attacker attacked, the user changed to another compromised address, noticed the issue, then changed to a whole new account address, but the attacker was able to change it back and withdraw the funds to the unprotected address).
https://github.com/Harpieio/contracts/blob/97083d7ce8ae9d85e29a139b1e981464ff92b89e/package.json#L23
Manual Review
Change to version 4.7.3
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/018-M
Lambda, Tomo, hickuphh3, IllIllI, defsec, sirhashalot
The unsafe casting of the recovered amount from uint256
to uint128
means the users’ funds will be lost.
logIncomingERC20()
has the recovered amount as type uint256
, but amountStored
is of type uint128
. There is an unsafe casting when incrementing amountStored
:
_erc20WithdrawalAllowances[_originalAddress][_erc20Address].amountStored += uint128(_amount);
It is thus possible for the amount recorded to be less than the actual amount recovered.
Loss of funds.
The user's balance is type(uint128).max = 2**128
, but the incremented amount will be zero.
amountStored
should be of type uint256
. Alternatively, use OpenZeppelin’s SafeCast library when casting from uint256
to uint128
.
Issue M-8: reduceERC721Fee function can not set fee when the NFT token ID is more than type(uint128).max
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/081-M
ak1
reduceERC721Fee
function can not set fee when the NFT token ID is more than type(uint128).max
The NFT token ID can be any value within uint 256.
As the reduceERC721Fee takes the _id
argument as uint 128
, when the reduceERC721Fee function is called with an NFT id that has above type(uint128).max
, the fee can not set to the expected NFT id.
High :
RC721Fee can not set fee when the NFT token ID value is more than type(uint128).max
Manual Review
Change the function argument for reduceERC721Fee
as shown below.
before fix :
function reduceERC721Fee(address _originalAddress, address _erc721Address, uint128 _id,
uint128 _reduceBy) external returns (uint128)
after fix:
function reduceERC721Fee(address _originalAddress, address _erc721Address, uint256 _id,
uint128 _reduceBy) external returns (uint128)
Source: https://github.com/sherlock-audit/2022-09-harpie-judging/tree/main/160-M
IllIllI
Nonces are not used in the signature checks
A nonce can prevent an old value from being used when a new value exists. Without one, two transactions submitted in one order, can appear in a block in a different order
If a user is attacked, then tries to change the recipient address to a more secure address, initially chooses an insecure compromised one, but immediately notices the problem, then re-submits as a different, uncompromised address, a malicious miner can change the order of the transactions, so the insecure one is the one that ends up taking effect, letting the attacker transfer the funds
Manual Review
Include a nonce in what is signed