You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelMediumA Medium severity issue.RewardA payout will be made for this issue
The protocol leaves a chance to list and delist asset types within the protocol. Position manager contract owners have the liberty to call the toggleKnownAsset function to add or remove an asset from the list of supported assets. When an asset is delisted, borrowers can still use the asset as a collateral against their positiondebt.
Vulnerability Detail
If an asset is delisted for any reason, it means such assets are no longer accepted by the protocol , and users can no longer deposit, nor use it as a collateral to back up loans. Although there is a check which succesfully ensures that delisted assets can no longer be deposited but borrowers with such assets can still use the assets as collateral. A similar issue has been reported previously on sentiment protocol, here is a link to the report; https://solodit.xyz/issues/m-11-delisted-assets-can-still-be-deposited-and-borrowed-against-by-accounts-that-already-have-them-sherlock-sentiment-sentiment-git
below is a foundry test to back up my claim
function test_PositionCanStillBorrowWithDelistedAsset() public {
vm.startPrank(user);
asset2.approve(address(positionManager), 10e18);
asset3.approve(address(positionManager), 10e18);
asset4.approve(address(positionManager), 10e18);
Action[] memory actions =newAction[](8);
(position, actions[0]) =newPosition(
user,
bytes32(uint256(0x123456789))
);
actions[1] =deposit(address(asset4), 2e18);
actions[2] =addToken(address(asset4));
actions[3] =deposit(address(asset2), 2e18);
actions[4] =addToken(address(asset2));
actions[5] =deposit(address(asset3), 2e18);
actions[6] =addToken(address(asset3));
actions[7] =borrow(fixedRatePool, 2.5e18);
positionManager.processBatch(position, actions);
riskModule.isPositionHealthy(position);
vm.stopPrank();
// Considering a case where the asset is delisted i.e toggled Unknown!
vm.startPrank(protocolOwner);
protocol.positionManager().toggleKnownAsset(address(asset2));
protocol.positionManager().toggleKnownAsset(address(asset3));
vm.stopPrank();
vm.startPrank(user);
// deposit 1e18 asset2, borrow 1e18 asset1
Action[] memory action =newAction[](2);
(position, actions[0]) =newPosition(
user,
bytes32(uint256(0x123456789))
);
action[0] =borrow(fixedRatePool, 0.25e18);
action[1] =borrow(fixedRatePool, 0.25e18);
positionManager.processBatch(position, action);
// The test passes,which means that an asset that has been delisited// can still be borrowed with.
}
Impact
This questions the integrity of the protocol as it is unfair for a group of users to have access to certain priviledges while others do not.
If an emergency warrants this scenario, consider a fair closure of all such position and prevent users from borrowing with delisted collateral by including a check against borrowing such asset
function borrow(address position, bytes calldata data) internal {
// data -> abi.encodePacked(uint256, uint256)
// poolId -> [0:32] pool to borrow from
// amt -> [32:64] notional amount to be borrowed
uint256 poolId = uint256(bytes32(data[0:32]));
uint256 amt = uint256(bytes32(data[32:64]));
// revert if the given pool does not exist
if (pool.ownerOf(poolId) == address(0)) revert PositionManager_UnknownPool(poolId);
+ if (!isKnownAsset[asset]) revert PositionManager_DepositUnknownAsset(asset);
// transfer borrowed assets from given pool to position
// trigger pool borrow and increase debt owed by the position
pool.borrow(poolId, position, amt);
// signals a borrow operation without any actual transfer of borrowed assets
// any checks needed to validate the borrow must be implemented in the position
Position(payable(position)).borrow(poolId, amt);
emit Borrow(position, msg.sender, poolId, amt);
}
sherlock-admin4
changed the title
Mysterious Green Stallion - Delisted assets still stands as a collateral
theweb3mechanic - Delisted assets still stands as a collateral
Sep 15, 2024
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelMediumA Medium severity issue.RewardA payout will be made for this issue
theweb3mechanic
Medium
Delisted assets still stands as a collateral
Summary
The protocol leaves a chance to list and delist asset types within the protocol. Position manager contract owners have the liberty to call the
toggleKnownAsset
function to add or remove an asset from the list of supported assets. When an asset is delisted, borrowers can still use the asset as a collateral against their positiondebt.Vulnerability Detail
If an asset is delisted for any reason, it means such assets are no longer accepted by the protocol , and users can no longer deposit, nor use it as a collateral to back up loans. Although there is a check which succesfully ensures that delisted assets can no longer be deposited but borrowers with such assets can still use the assets as collateral. A similar issue has been reported previously on sentiment protocol, here is a link to the report; https://solodit.xyz/issues/m-11-delisted-assets-can-still-be-deposited-and-borrowed-against-by-accounts-that-already-have-them-sherlock-sentiment-sentiment-git
below is a foundry test to back up my claim
Impact
This questions the integrity of the protocol as it is unfair for a group of users to have access to certain priviledges while others do not.
Code snippet
https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/PositionManager.sol#L384
Tool used
Foundry
Manual Review
Recommendation
If an emergency warrants this scenario, consider a fair closure of all such position and prevent users from borrowing with delisted collateral by including a check against borrowing such asset
function borrow(address position, bytes calldata data) internal { // data -> abi.encodePacked(uint256, uint256) // poolId -> [0:32] pool to borrow from // amt -> [32:64] notional amount to be borrowed uint256 poolId = uint256(bytes32(data[0:32])); uint256 amt = uint256(bytes32(data[32:64])); // revert if the given pool does not exist if (pool.ownerOf(poolId) == address(0)) revert PositionManager_UnknownPool(poolId); + if (!isKnownAsset[asset]) revert PositionManager_DepositUnknownAsset(asset); // transfer borrowed assets from given pool to position // trigger pool borrow and increase debt owed by the position pool.borrow(poolId, position, amt); // signals a borrow operation without any actual transfer of borrowed assets // any checks needed to validate the borrow must be implemented in the position Position(payable(position)).borrow(poolId, amt); emit Borrow(position, msg.sender, poolId, amt); }
Duplicate of #282
The text was updated successfully, but these errors were encountered: