search

sherlock-audit / 2023-04-ajna-judging

4 stars 3 forks source link

PRAISE - Incorrect rounding of collateralAmount in take() function #76

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

PRAISE

high

Incorrect rounding of collateralAmount in take() function

Summary

Vulnerability Detail

According openzeppelin's docs for the construction of ERC721 tokens, it is stated that " unlike ERC20, ERC721 lacks a decimals field, since each token is distinct and cannot be partitioned." you'll see that here --https://docs.openzeppelin.com/contracts/3.x/erc721#constructing_an_erc721_token_contract

But in the take() function in ERC721Pool.sol, when _transferFromPoolToAddress() is called, the function divides result.collateralAmount which is the collateral amount to send to taker by 1e18 

  // transfer rounded collateral from pool to taker
        uint256[] memory tokensTaken = _transferFromPoolToAddress(
            callee_,
            borrowerTokenIds[borrowerAddress_],
            result.collateralAmount / 1e18
        );

Since ERC721 tokens lacks a decimal field this will result in sending miscalculated and incorrect collateral amount to taker.

Impact

miscalculated and incorrect collateral amount will be sent to taker

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC721Pool.sol#L467-L472

Tool used

Manual Review

Recommendation

since ERC721 tokens lacks a decimal field maybe don't divide by 1e18

Love4codes commented 1 year ago

Escalate for 10 USDC

ERC721 doesn't have a decimal field, so there's no need to divide collateral amount to send to taker by 1e18 This will cause miscalculated and incorrect collateral amount to be sent to taker

sherlock-admin commented 1 year ago

Escalate for 10 USDC

ERC721 doesn't have a decimal field, so there's no need to divide collateral amount to send to taker by 1e18 This will cause miscalculated and incorrect collateral amount to be sent to taker

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0xffff11 commented 1 year ago

Issue seems valid. Thoughts? @grandizzy

grandizzy commented 1 year ago

The issue is not valid, since the same code / accounting is used for both ERC20 and ERC721 pool we actually store NFTs as WADs (that is for 1 NFT pledged we record 1e18 as pledged collateral) - see https://github.com/ajna-finance/ajna-core/blob/main/src/ERC721Pool.sol#L166 for pledge collateral https://github.com/ajna-finance/ajna-core/blob/main/src/ERC721Pool.sol#L245 for pull collateral https://github.com/ajna-finance/ajna-core/blob/main/src/ERC721Pool.sol#L461 for take collateral

Therefore the calculated number of tokens to transfer should be collateral / 1e18

Sample of unit test:

dmitriia commented 1 year ago

18 dp precision is a convention used within the protocol. It's actually one of the best approaches to handle decimals related surfaces in general.

Love4codes commented 1 year ago

18 dp precision is a convention used within the protocol. It's actually one of the best approaches to handle decimals related surfaces in general.

Okay boss, i have taken note.

hrishibhat commented 1 year ago

Result: Invalid Unique Considering this a non-issue based on the above Sponsor and Lead Watson comments.

sherlock-admin commented 1 year ago

Escalations have been resolved successfully!

Escalation status: