Astaria contest - wallstreetvilkas's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L114-L169

Vulnerability details

Impact Detailed description of the impact of this finding.

Calling the function safeTransferFrom() and passing in a custom, attacker controlled payment token allows the malicious actor to perform reentrancy. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L153-L158 the following function clears the attacker of all debt, which is costless to the attacker because the token is worthless. After the debt is cleared, the following check is performed: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L160, this is where the attacker reenters. Attacker calls the releaseToAddress() in collateral token contract, and then he calls into a malicious erc721 contract, the malicious erc721 contract then calls into this function: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L553, and because the idToUnderlying is deleted, it can overwrite it and it won't revert and the nft is sent to the attacker, and he gets to keep it.

Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Create a malicious contract. In the contract, define a custom balanceOf() function which calls into CollateralToken's releaseToAddress() passing in the collateralId and calls the custom, attacker controlled erc721 contract. Because the balanceOf() of our malicious contract is called not once, but twice, make a variable that stores which time the function is being called like this pseudo code:

uint var; erc721 erc721Contract; //this is our malicious erc721 contract which calls into onErc721Received() in collateralToken, which overwrites the underlying id function balanceOf() public returns (uint){ if(var == 0){ var = var + 1; return anyAmountToPassTheFirstCheck; } else{ releaseToAddress() //first call erc721Contract.attack() //this is our custom defined erc721. the attack function will call this function https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L553 this overwrites our deleted underlyingid with our custom contract, so nothing reverts

} }

Through this malicious contract, call safeTransferFrom() function in ClearingHouse pass in, encoded metadata (which contains our malicious contract's address uint256(bytes32(maliciousContractAddr)), the collateralId of the nft and then finally the two arguments to and from (it doesn't matter what they are, because they are unused throughout the code). This invokes the balanceOf twice. The second time we reenter, release the nft to the attacker, call into our erc721 contract which overwrites the underlyingid so that it exists when house invokes the settleAuction() function. The nft is transfered to the attacker.

Tools Used

manual review

Recommended Mitigation Steps

ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse( paymentToken, collateralId, payment - liquidatorPayment, s.auctionStack.stack ); if (ERC20(paymentToken).balanceOf(address(this)) > 0) { ERC20(paymentToken).safeTransfer( ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId), ERC20(paymentToken).balanceOf(address(this)) ); } ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId);

This is the vulnerable code before, I recommend you change it to this:

ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse( paymentToken, collateralId, payment - liquidatorPayment, s.auctionStack.stack ); ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId); if (ERC20(paymentToken).balanceOf(address(this)) > 0) { ERC20(paymentToken).safeTransfer( ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId), ERC20(paymentToken).balanceOf(address(this)) ); }

At this point it's too late for the attacker to reenter as nothing can be gained, but another issue still stands, the attacker can pay off debt with worthless tokens. Consider adding a mapping of whitelisted, trusted tokens to pay with, or simply force the user to pay off debt in the tokens that they borrowed.