NameWrapper._transferAndBurnFuses() allows reentrancy on onERC1155Received callback

Lines of code Vulnerability details Impact When calling the internal function transferAndBurnFuses in NameWrapper.setSubnodeOwner or NameWrapper.setSubnodeRecord , ERC1155Fuse.transfer is called before setFuses which creates a reentrancy opportunity when newOwner is a contract, which may allow a...

Transfer ETH by using transfer() may cause this transaction to fail

Lines of code Vulnerability details Impact Transfer ETH by using transfer may cause this transaction to fail. In EIP-1884: In many cases, a recipient of ether from a CALL will want to issue a LOG. The LOG operation costs 375 plus 375 per topic. If the LOG also wants to do an SLOAD, this change ma...

[PNM-003] The preimage DB (i.e., NameWrapper.names) can be maliciously manipulated/corrupted

Lines of code Vulnerability details Description By design, the NameWrapper.names is used as a preimage DB so that the client can query the domain name by providing the token ID. The name should be correctly stored. To do so, the NameWrapper record the domain's name every time it gets wrapped. And...

Usage of deprecated transfer to send ETH

Lines of code%20%7B-,payablemsg.sender.transfer,-msg.valueL183 Vulnerability details 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...

Users can skirt fuses on subnodes

Lines of code Vulnerability details Impact If users are granted subnode ownership through setSubnodeRecord or setSubnodeOwner in NameWrapper.sol, and that node is owned by the NameWrapper contract in the ENS registry and the unwrap fuse is not set, then attackers can reset flags and do whatever...

[PNM-002] The expiry of the parent node can be smaller than the one of a child node, violating the guarantee policy

Lines of code Vulnerability details Description By design, the child node's expiry can only be extended up to the parent's current one. Adding these restrictions means that the ENS users only have to look at the name itself's fuses and expiry without traversing the hierarchy to understand what...

ETHRegistrarController.sol#register() preforms a low-level call to a user desired target.data

Lines of code Vulnerability details When using low-level call to target that entered by the caller we need to make sure it will only do what it meant to do without any extra power. The ETHRegistrarController.solregister takes a resolver and data to preform a low-level call to the resolver. Howeve...

When setSubnodeOwner transfers ownership of sub-domain the new owner can perform actions before fuses are burned

Lines of code Vulnerability details Impact Function NameWrapper.setSubnodeOwner can be used to transfer ownership of a sub-domain to a new owner and, at the same time, burn fuses. A possible use-case could be that a domain owner wants to transfer ownership of the sub-domain but burn fuses in orde...

Some implementation mistakes in dnssec-oracle/BytesUtils.compare

Lines of code Vulnerability details Impact There are some implementation mistakes in dnssec-oracle/BytesUtils.compare. There should be a sanity check for offsets and lens if shortest 32 is not a correct condition. It should check the size of the last block. shortest is the total size of the bytes...

Use Call Instead of Transfer for Address Payable

Lines of code Vulnerability details Impact It is recommended to avoid the usage of payable.transfer, since it can cause the transaction to fail when the user is accessing this function with a smart contract and: 1. does not have payable function 2. have a payable function but spends more than 230...

NameWrapper: parent can change owner of subnode by minting

Lines of code Vulnerability details Impact HIGH - bypassing PARENTCANNOTCONTROL fuse As discussed in the discord, bypassing fuse is considered high, thus reports as high impact Condition for the parent for this exploit: should be able to unwrap: no CANNOTUNWRAP fuse on the parent Or alternatively...

Use SAFETRANSFERFROM() instead of TRANSFERFROM() for ERC721 TRANSFERS

Lines of code Vulnerability details Impact In NameWrapper.sol the wrapETH2LD and unwrapETH2LD functions call transferFrom on a ERC721 token. This does not ensure that the token is not sent to an address that is not able to properly support it which could result in the loss of the token. This is...

CALL() should be used instead of TRANSFER() on an address payable

Lines of code Vulnerability details Impact The use of the deprecated transfer function for an address will inevitably make the transaction fail when: 1 The claimer smart contract does not implement a payable function. 2 The claimer smart contract does implement a payable fallback which uses more...

Reentrancy from _transferAndBurnFuses

Lines of code Vulnerability details Impact Reentrancy attack Proof of Concept The function transferAndBurnFuses is not performing Checks-Effects-Interactions pattern, and updates fuses after NFTs are transfered. An attacker can reenter because transfer is performing a doSafeTransferAcceptanceChec...

ETHRegistrarController's register and renew use payable.transfer calls with an arbitrary receiver

Lines of code Vulnerability details ETHRegistrarController's register and renew transfer out remainder native tokens via payableto.transfer call. This is unsafe as transfer has hard coded gas budget and can fail when msg.sender is a smart contract. Such transactions will fail for smart contract...

Reentrancy for function call before state update

Lines of code Vulnerability details Impact An external call "transfer" is made before updating state data through "setFuses" and "setFuses" does not depend on any data from "transfer". . Proof of Concept Reentrancy is not only an effect of Ether transfer but of any function call on another...

Unchecked Return Value for transfer & transferFrom call

Lines of code Vulnerability details Impact It is good to add a require statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silen...

Incorrect implementation of RRUtils.serialNumberGte

Lines of code Vulnerability details Impact Comparing serial numbers should follow RFC1982 due to the possibility of numbers wrapping around. RRUtils.serialNumberGte tried to follow the RFC but failed to do so, leading to incorrect results in comparison. Proof of Concept For a serial number i1 to ...

NameWrapper: parent can bypass PARENT_CANNOT_CONTROL

Lines of code Vulnerability details Impact HIGH - bypassing PARENTCANNOTCONTROL fuse As discussed in the discord, bypassing fuse is considered high, thus it is reported as high impact Conditions for the parent for this exploit: should be able to unwrap: no CANNOTUNWRAP fuse on the parent node...

Call() should be used instead of transfer() on an address payable

Lines of code Vulnerability details Impact The use of the deprecated transfer function for an address will inevitably make the transaction fail when : 1. The withdrawer smart contract does not implement a payable fallback function. 2. The withdrawer smart contract implements a payable fallback...

call() should be used instead of transfer() on an address payable

Lines of code Vulnerability details Impact The use of the deprecated transfer function for an address will inevitably make the transaction fail when the caller is a smart contract and: 1. Does not implement a payable function. 2. Implements a payable fallback which uses more than 2300 gas unit. 3...

DNSSECImpl.verifySignature compares strings incorrectly, allowing malicious zones to forge DNSSEC trust chain

Lines of code Vulnerability details Impact DNSSEC allows parent zones to sign for its child zones. To check validity of a signature, RFC4034 3.1.7 requires the Signer's Name in any RRSIG RDATA to contain the zone of covered RRset. This requirement is reasonable since any child zone should be...

Parent domain owner can steal ownership and clear any fuses for any sub-domain if CANNOT_UNWRAP is not burnt on his own domain

Lines of code Vulnerability details Impact There is a general incorrect logic of burning fuses throughout NameWrapper, which allows parent domain owner to burn subdomain fuses including PARENTCANNOTCONTROL regardless of parent domain's own fuses only subdomain fuses are checked, parent fuses are...

It is possible to create fake ERC1155 NameWrapper token for subdomain, which is not owned by NameWrapper

Lines of code Vulnerability details Impact Due to re-entrancy possibility in NameWrapper.transferAndBurnFuses called from setSubnodeOwner and setSubnodeRecord, it is possible to do some stuff in onERC1155Received right after transfer but before new owner and new fuses are set. This makes it...

Vault owners can grief auctioneers by cancelling auctions

Lines of code Vulnerability details Since auctioneers are only paid keeper incentives on successful bids, a malicious vault owner can grief auctioneers by creating a risky vault, luring an auctioneer to start an auction, and then immediately canceling it. This is a somewhat elaborate and risky...

If baseJoin's available balance is greater than callers' amount no underlying will be transferred.

Lines of code Vulnerability details Impact If callers' amount is greater than baseJoin's available balance no underlying will be transferred but caller still gets liquidatorCut. This may also negatively impact the accounting since its updated under the assumption that the caller will transfer...

There is no boundaries for starting an auction.

Lines of code Vulnerability details Impact The team has brought this contest to be fairer to their users in terms of liquidation mechanism. Looking through this perspective, it can be developed more on the process to be much fairer to the users in volatile market conditions. Proof of Concept The...

Auctioneer could be left without reward

Lines of code Vulnerability details Impact Reward amount to whomever started the auction auctioneerCut counts at the moment when auction is bought, based on current auctioneerReward value using calcPayout function. ... if auction.auctioneer != to auctioneerCut = liquidatorCut.wmulauctioneerReward...

The auction doesn't check who puts a vault up for auction and can lead to a denial of service attack, and other actions beneficiary an attacker

Lines of code Vulnerability details Impact The permission for whoever can set an undercollaterized vault up for auction is not checked, an arbitrary user can set any vault for auction, set themselves up as the auctioneer and also get the reward. The real owners of the vault wont be able to their...

User does not get commission

Lines of code Vulnerability details Impact If Auctioned Vault debt is cleared from external source depositing extra collateral, auction can be cancelled prematurely. In this case auctioneer loses his commission. User must be given commission once auction starts at auctionStarted. Note Also it see...

Vaults are not locked properly

Lines of code Vulnerability details Impact Even though after Auction starts, Vault ownership moves to Witch but still Cauldron Admin can perform operations on this Vault. This includes transferring funds from a Vault which has live Auction ongoing Proof of Concept 1. Auction is started on Vault i...

auction.start might be zero after a new auction is created.

Lines of code Vulnerability details Impact Currently several functions consider the auction is valid when auction.start 0 and they don't work properly when auction.start = 0. Proof of Concept In this part, auction.start = 0 when block.timestamp = 232 exactly. Of course, this is an edge case and...

There is no method to unignore any ignored pair.

Lines of code Vulnerability details Impact The Witch contract uses setIgnoredPair function to prevent the liquidation of accepted pairs on the governance level. However, there is no method to remove these pairs from the ignoredPairs mapping. Proof of Concept Imagine there are vaults with UST/fyUS...

CHECK VALID ADDRESS for _auctionStarted

Lines of code%20internal%20virtual%20%7BL214 Vulnerability details Impact Should check for valid address before giving so it can revert if it isn't. Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the...

Reduce the incentive of self liquidation

Lines of code Vulnerability details Reduce the incentive of self liquidation Impact In some situations the borrower may have incentive to self liquidate. For example: May pay less interest Redeem early and reduce the loss or the fee React to abrupt market volitility Other cases to circumvent some...

limits_.sum is not increased yet when performing limit check. Collateral may exceed the limit.

Lines of code Vulnerability details Impact limits.sum is not increased yet when performing limit check. Collateral may exceed the limit. There is a limit on how much collateral can be concurrently put at auction, but it is a soft limit. If the limit has been surpassed, no more vaults of that...

Someone can create non-liquidatable auction if the collateral asset fails on transferring to address(0)

Lines of code Vulnerability details Impact might lead to systematic debt. Cause errors for liquidators to run normally. Proof of Concept In the function auction, there is on input validation around whether the to is address0 or not. and if the auctioneerReward is set to an value 0 as default, eac...

Vault blacklisting is required

Lines of code Vulnerability details Impact Add a way to blacklist vault from going in Auction. There may be cases where owner does not want a particular Vault to be Auctioned even if it is undercollateralized as shown in POC. Due to lack of any blacklisting all Vault are treated equally and can g...

On the auction function start time is not set

Lines of code Vulnerability details Impact During the code review, It has been noticed that auction start time block.timestamp is not set. All pay functions are checking If the auction.start is more than zero. From that reason, auction start time should be set at auction function. Proof of Concep...

Auctions can be started for same undercollateralized vault using different Witch contracts, which causes vault collateral loss

Lines of code Vulnerability details Impact Given that there are two deployed Witch contracts, two auctioneers are calling Witch 1 and Witch 2's auction functions for the same vault at similar times. Both transactions are included in the same block for which the governances of these Witch contract...

The whole collateral can be liquidatedThe whole collateral can be liquidated

Lines of code Vulnerability details The whole collateral can be liquidated Impact According to the current mechanism, if the proportion is set to 100%, and some user's profile barely trigger the liquidation threshold, the whole collateral could be liquidated all at once. However, the user's profi...

Caller can receive liquidatorCut without transferring underlying when calling payBase()

Lines of code Vulnerability details Impact If baseJoin's available balance is greater than callers' amount no underlying will be transferred but caller still gets liquidatorCut. This may also negatively impact the accounting since its updated under the assumption that the caller will transfer...

Validation check for payFYToken and payBase functions to allow only valid users.

Lines of code Vulnerability details Impact This will prevent from calling the functions by invalid users. Unnecessary condition verification. Proof of Concept Tools Used Manual code review Recommended Mitigation Steps At the start of the functions, add validation check such that only the user who...

Collateral limit can be bypassed in auction

Lines of code Vulnerability details Collateral limit could be bypassed, allowing more collateral to be auctioned concurrently than intended by governance. // There is a limit on how much collateral can be concurrently put at auction, but it is a soft limit. // If the limit has been surpassed, no...

Auctioneer Cut calculated in different order of magnitude

Lines of code Vulnerability details Impact Auctioneer fee is calculated in different order of magnitude. Proof of Concept As your are defining auctioneerReward in 1e18 basis point being 1e18 100% you need to divide by 1e18 when calculating percentage. function setAuctioneerRewarduint128...

Liquidator cut is calculated incorrectly

Lines of code Vulnerability details Impact User can call payBase function to pay debt in return for collateral. But due to incorrect calculation User will obtain lesser collateral than what he should actually receive causing immediate permanent loss Proof of Concept 1. User calls payBase function...

Possible casting overflow in _updateAccounting function

Lines of code Vulnerability details Impact In the updateAccounting function, the inkOut and artIn parameters are cast from type uint256 to uint128. However, since the explicit cast does not ensure the value fits the uint128 data type i.e., not ensuring value = typeuint128.max, casting overflows a...

Incorrect payout calculation due to a division before multiplication

Lines of code Vulnerability details Impact Reverse dutch auction price is calculated incorrectly Proof of Concept In the function calcPayout which calculates reverse dutch auction according to the formula inkOut = artIn / totalArt totalInk p + 1 - p t t is always zero because you divide before...

Incorrect validation check results with always initial proportion to the liquidator.

Lines of code Vulnerability details Impact The liquidator will get the initial proportion of value irrespective how long the time passed. Proof of Concept if duration == typeuint32.max in above condition check need to be checked with "elapsed" not with "duration" . Since it was mentioned that...

Closing auction return value Of cauldron is not checked

Lines of code Vulnerability details Impact When the auction is ended, cauldron gaves vault id with related parameters. However, the return value is not checked on the cauldron. If the cauldron does not revert, the protocol can delete auction without functionality. Proof of Concept Tools Used Code...