Platform: Code4rena
Start Date: 18/02/2022
Pot Size: $125,000 USDC
Total HM: 13
Participants: 24
Period: 14 days
Judge: GalloDaSballo
Total Solo HM: 6
Id: 88
League: ETH
Rank: 7/24
Findings: 4
Award: $5,476.18
🌟 Selected for report: 2
🚀 Solo Findings: 0
1618.2353 USDC - $1,618.24
https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L120-L124 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206
Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to ERC20Upgradeable, their function signatures do not match and therefore the calls made will revert.
The code as currently implemented does not handle these sorts of tokens properly when they're deposited or withdrawn. Tether cannot be deposited because depositing requires a cast and a call to transferFrom() which will revert. If a token has a different signature for transfer() but the correct one for transferFrom() the user's tokens will be stuck.
ERC20Upgradeable(erc20OnMainnet).transferFrom( msg.sender, address(this), amount ),
function _removeTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private { transferredAmount[schainHash][erc20Token] -= amount; }
ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),
Code inspection
Use OpenZeppelin’s SafeERC20Upgradeable's safeTransfer() and safeTransferFrom() instead
#0 - cstrangedk
2022-03-04T01:52:52Z
Reference/duplicate disputed #8, #9, #10, #12, #19.
Disputed as SKALE Chain owner can customize new depositbox contract to use safetransfer functions if needed, and use registerExtraContract().
#1 - GalloDaSballo
2022-06-01T16:49:25Z
Dup of #8
🌟 Selected for report: IllIllI
Also found by: gzeon, kirk-baird
1618.2353 USDC - $1,618.24
https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxEth.sol#L138-L142 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L196-L200 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol#L183-L187 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L261-L271
Once a chain has been killed the chain owner is able to call getFunds() on each of the deposit boxes and transfer funds/tokens wherever he/she wishes
Even if the owner is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. See this example where a similar finding has been flagged as a high-severity issue. I've downgraded these instances to be a medium since it requires cooperation of the IMA mainnet admin.
function getFunds(string calldata schainName, address payable receiver, uint amount) external override onlySchainOwner(schainName) whenKilled(keccak256(abi.encodePacked(schainName)))
function getFunds(string calldata schainName, address erc20OnMainnet, address receiver, uint amount) external override onlySchainOwner(schainName) whenKilled(keccak256(abi.encodePacked(schainName)))
function getFunds(string calldata schainName, address erc721OnMainnet, address receiver, uint tokenId) external override onlySchainOwner(schainName) whenKilled(keccak256(abi.encodePacked(schainName)))
function getFunds( string calldata schainName, address erc1155OnMainnet, address receiver, uint256[] memory ids, uint256[] memory amounts ) external override onlySchainOwner(schainName) whenKilled(keccak256(abi.encodePacked(schainName)))
Code inspection
Add a long time lock for killing so users have plenty of time to get their funds out before a kill
#0 - DimaStebaev
2022-03-11T14:17:39Z
This is not a vulnerability. We explicitly added this functionality to prevent loosing of funds in case of technical problems with SKALE chain. It requires SKALE chain owner and SKALE foundation cooperation to run this mechanism. It is going to be removed eventually after some time of stable work.
#1 - GalloDaSballo
2022-06-01T16:48:43Z
While I empathize with the sponsor's side that the function is meant as a security mechanism, it does allow the chainOwner to pull all funds and transfer them to their own wallet.
Because this is contingent on Admin Privilege, I believe Medium Severity to be appropriate
🌟 Selected for report: 0x1f8b
Also found by: IllIllI, cmichel, gzeon, kirk-baird
786.4623 USDC - $786.46
https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L120-L124 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206
Some ERC20 tokens, such as Tether (USDT), allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance.
The current implementation does not work properly with fee-on-transfer tokens when depositing/withdrawing the tokens, which will lead to either the calls reverting or some users being able to claim more than they should be, with the final users trying to withdraw from the lockbox being stuck with all of the fees.
When a token is transferred in the amount is stored, and this amount may be wrong:
function _saveTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private { transferredAmount[schainHash][erc20Token] += amount; }
Later when the user asks for it back they're able to ask for the original amount even though the contract doesn't hold that amount
_removeTransferredAmount(schainHash, message.token, message.amount); require( ERC20Upgradeable(message.token).transfer(message.receiver, message.amount),
function _removeTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private { transferredAmount[schainHash][erc20Token] -= amount; }
If other users have deposited without withdrawing then the user will be able to withdraw more than they should be able to. The post-kill admin withdrawal has the same issue:
ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),
Code inspection
One way to work around the issue is to measure the contract's balance right before and after the asset-transferring functions, and using the difference rather than the stated transferred amount.
#0 - cstrangedk
2022-03-04T00:40:51Z
Duplicate and disputed of #42, #53, #20
#1 - GalloDaSballo
2022-06-01T16:50:12Z
Dup of #50
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, CertoraInc, TerrierLover, Tomio, WatchPug, d4rk, gzeon, kenta, kyliek, m_smirnova2020, rfa, robee, saian, ye0lde
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
mapping(bytes32 => mapping(address => bool)) internal deprecatedRegistryContracts;
mapping(bytes32 => mapping(address => bool)) private _deprecatedSchainToERC20;
mapping(bytes32 => bool) public activeEthTransfers;
mapping(bytes32 => mapping(address => bool)) private _deprecatedSchainToERC1155;
mapping(bytes32 => mapping(address => bool)) private _deprecatedSchainToERC721;
mapping(bytes32 => bool) private _interchainConnections;
bool public override messageInProgress;
mapping(bytes32 => bool) private _automaticDeploy;
mapping(address => mapping(bytes32 => bool)) public activeUsers;
abi.encode() is less efficient than abi.encodePacked()data = abi.encode(receiver, tokenId, tokenURI);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
return abi.encode(message);
require() strings longer than 32 bytes cost extra gasrequire(hasRole(EXTRA_CONTRACT_REGISTRAR_ROLE, msg.sender), "EXTRA_CONTRACT_REGISTRAR_ROLE is required");
require(hasRole(CONSTANT_SETTER_ROLE, msg.sender), "Not enough permissions to set constant");
require(!_getRegistryContracts()[bytes32(0)].contains(extraContract), "Extra contract is already registered");
require(connectedChains[dstChainHash].inited, "Destination chain is not initialized");
require(connectedChains[targetChainHash].inited, "Destination chain is not initialized");
require(!_getRegistryContracts()[chainHash].contains(extraContract), "Extra contract is already registered");
"Extra contract is already registered for all chains"
"Destination contract is not a contract"
"Sender contract is not registered"
"DepositBox was not approved for ERC20 token"
"Not enough money to finish this transaction"
"DepositBox was not approved for ERC1155 token"
"DepositBox was not approved for ERC1155 token Batch"
return bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)"));
return bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"));
"DepositBox was not approved for ERC721 token"
revert("Already killed or incorrect sender");
require(address(newCommunityPoolAddress) != address(0), "CommunityPool address has to be set");
"Not enough permissions to register extra contract"
require(schainHash != MAINNET_HASH, "Schain hash can not be equal Mainnet");
"Not enough permissions to register extra contract"
require(schainHash != MAINNET_HASH, "Schain hash can not be equal Mainnet");
require(_checkSchainBalance(fromSchainHash), "Schain wallet has not enough funds");
"Starting counter is not equal to incoming message counter");
require(keccak256(abi.encodePacked(schainName)) != MAINNET_HASH, "Schain id can not be equal Mainnet");
"Sender contract is not registered"
"SKALE chain name cannot be Mainnet"
require(contractReceiver != address(0), "Incorrect address of contract receiver on Schain");
"../schain/tokens/ERC721OnChain.sol";
"../schain/tokens/ERC721OnChain.sol";
require(getMessageType(data) == MessageType.TRANSFER_ERC20, "Message type is not ERC20 transfer");
"Message type is not ERC20 transfer and total supply"
"Message type is not ERC20 transfer with token info"
require(getMessageType(data) == MessageType.TRANSFER_ERC721, "Message type is not ERC721 transfer");
"Message type is not ERC721 transfer with token info"
require(getMessageType(data) == MessageType.INTERCHAIN_CONNECTION, "Message type is not Interchain connection");
require(getMessageType(data) == MessageType.TRANSFER_ERC1155, "Message type is not ERC1155 transfer");
"Message type is not ERC1155AndTokenInfo transfer"
"Message type is not ERC1155Batch transfer"
"Message type is not ERC1155BatchAndTokenInfo transfer"
> 0 costs more gas than != 0 when used on uints in a require() statementrequire(approveTransfers[msg.sender] > 0, "User has insufficient ETH");
<array>.length should not be looked up in every loop of a for-loopEven memory arrays incur the overhead of bit tests and bit shifts to calculate the array length
for (uint256 i = 0; i < messages.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < ids.length; i++) {
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < tokens.length; i++) {
for (uint i = 0; i < schainContracts.length; i++) {
for (uint i = 0; i < length; i++) {
for (uint i = 0; connected && i < length; i++) {
for (uint256 i = 0; i < contracts.length; i++) {
for (uint256 i = 0; i < messages.length; i++) {
++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loopsfor (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < messages.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < ids.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint i = 0; i < schainContracts.length; i++) {
for (uint i = 0; i < length; i++) {
for (uint i = 0; connected && i < length; i++) {
for (uint256 i = 0; i < contracts.length; i++) {
for (uint256 i = 0; i < messages.length; i++) {
++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)for (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < messages.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < ids.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = from; i < to; i++) {
for (uint i = 0; i < schainContracts.length; i++) {
for (uint i = 0; i < length; i++) {
for (uint i = 0; connected && i < length; i++) {
for (uint256 i = 0; i < contracts.length; i++) {
for (uint256 i = 0; i < messages.length; i++) {
for (uint256 i = 0; i < messages.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < tokens.length; i++) {
for (uint256 i = 0; i < ids.length; i++) {
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < ids.length; i++)
for (uint256 i = 0; i < tokens.length; i++) {
for (uint i = 0; i < schainContracts.length; i++) {
for (uint i = 0; i < length; i++) {
for (uint i = 0; connected && i < length; i++) {
for (uint256 i = 0; i < contracts.length; i++) {
uint notReimbursedGas = 0;
for (uint256 i = 0; i < messages.length; i++) {
#0 - yavrsky
2022-03-14T15:15:08Z
Only marginal gas improvements.
#1 - GalloDaSballo
2022-04-28T15:35:02Z
Per the rationale 100 gas per bool used 8 * 100 = 800 (removed the non mapping as a toggle probably can't be optimized)
In lack of any detail am non counting this, would recommend the warden to always explain why something works
Per the discussion in #30 2500 per string, will also cap at 25 findings, some of the linked strings are random strings 25 * 2500 = 62500
3 gas
3 gas per check * 12 times = 36
22 gas per instance * 16 = 352
Will ignore per the above
3 gas per instance * 13 = 39
#2 - GalloDaSballo
2022-04-28T15:42:26Z
Total: 63730